refactor: new interface for ExportResult #1569 #1643

vmarchaud · 2020-10-31T20:47:57Z

There is 2 goals for this PR:

Remove FAILED_RETRYABLE result since it has been removed from the spec and we expect exporter to handle this.
Add an optional error inside the result so the error is correctly handled at one place (on the Controller for metrics and SpanProcessor for spans). Previously every exporter had to use the globalErrorHandler to report the error which made multiple errors appears (one from the exporter and one for the upstream component that called it)

The design is the same as the Golang implementation: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md#go-spanexporter-interface

Fixes #1569

codecov · 2020-10-31T21:18:26Z

Codecov Report

Merging #1643 into master will increase coverage by 0.11%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #1643      +/-   ##
==========================================
+ Coverage   91.19%   91.31%   +0.11%     
==========================================
  Files         165      165              
  Lines        5066     5066              
  Branches     1037     1044       +7     
==========================================
+ Hits         4620     4626       +6     
+ Misses        446      440       -6

Impacted Files	Coverage Δ
...etry-exporter-prometheus/src/PrometheusExporter.ts	`90.16% <50.00%> (ø)`
...ckages/opentelemetry-exporter-jaeger/src/jaeger.ts	`86.84% <60.00%> (+1.12%)`	⬆️
...ges/opentelemetry-metrics/src/export/Controller.ts	`79.16% <66.66%> (+33.71%)`	⬆️
...telemetry-tracing/src/export/BatchSpanProcessor.ts	`90.90% <66.66%> (-1.28%)`	⬇️
...elemetry-tracing/src/export/SimpleSpanProcessor.ts	`82.75% <75.00%> (-2.43%)`	⬇️
packages/opentelemetry-core/src/ExportResult.ts	`100.00% <100.00%> (ø)`
...ry-exporter-collector/src/CollectorExporterBase.ts	`91.30% <100.00%> (-0.54%)`	⬇️
...elemetry-exporter-zipkin/src/platform/node/util.ts	`96.66% <100.00%> (-0.31%)`	⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts	`100.00% <100.00%> (ø)`
...emetry-metrics/src/export/ConsoleMetricExporter.ts	`62.50% <100.00%> (ø)`
... and 3 more

dyladan

LGTM. One quick note about returning an error matching the go impl: in go this is the only way to report an error as there is no throw/try/catch. I'm still fine with it though.

dyladan · 2020-11-02T15:35:06Z

packages/opentelemetry-exporter-collector/src/platform/browser/util.ts

@@ -72,7 +72,7 @@ export function sendWithXhr(
        onSuccess();
      } else {
        const error = new collectorTypes.CollectorExporterError(
-          xhr.responseText,
+          `Failed to export with XHR (status: ${xhr.status})`,


What is the reason for this change?

Depending on what the collector report (which itself depend on upstream exporters) you can have a empty text. I prefered to opt for this generic text so user get an error message every time.

xhr.status already is passed to CollectorExporterError. Maybe it is better to have xhr.responseText || "Failed to export with XHR"?

vmarchaud · 2020-11-02T15:47:43Z

One quick note about returning an error matching the go impl: in go this is the only way to report an error as there is no throw/try/catch. I'm still fine with it though.

I agree that it would be better if we could only use throw/try/catch (with async/await), however since we have a callback interface i prefered to keep the callback system (which i think is required in the spec ?).

dyladan · 2020-11-02T20:20:26Z

One quick note about returning an error matching the go impl: in go this is the only way to report an error as there is no throw/try/catch. I'm still fine with it though.

I agree that it would be better if we could only use throw/try/catch (with async/await), however since we have a callback interface i prefered to keep the callback system (which i think is required in the spec ?).

I agree. I was just mentioning it for completeness and for others who might question it.

obecny

one minor thing, other that that lgtm.

Should this have a breaking label ?

obecny · 2020-11-03T23:03:14Z

packages/opentelemetry-exporter-collector/test/browser/CollectorMetricExporter.test.ts

-
+        collectorExporter.export(metrics, result => {
+          assert.deepStrictEqual(result.code, ExportResultCode.FAILED);
+          assert.ok(result.error?.message.includes('cannot send'));


the message is still sendBeacon - cannot send so why shorter check then ?

i just rewrote the code without thinking about what was in place, i will add it back

obecny · 2020-11-03T23:04:07Z

packages/opentelemetry-exporter-collector/test/browser/CollectorTraceExporter.test.ts

-
+        collectorTraceExporter.export(spans, result => {
+          assert.deepStrictEqual(result.code, ExportResultCode.FAILED);
+          assert.ok(result.error?.message.includes('cannot send'));


here the same sendBeacon - cannot send ?

vmarchaud · 2020-11-04T07:27:35Z

Should this have a breaking label ?

Yeah i believe, specially for vendor exporters.

vmarchaud added 7 commits October 31, 2020 21:40

refactor: use interface with optional error for ExportResult

a7199e3

refactor: use new export result for exporter-collector

b2d4d10

refactor: use new export result for exporter-jaeger

2b45bc7

refactor: use new export result for exporter-prometheus

67831bd

refactor: use new export result for exporter-zipkin

a3cca78

refactor: use new export result for metrics sdk

acbf14a

refactor: use new export result for tracing sdk

06f3b7a

vmarchaud requested review from dyladan, legendecas, markwolff, mayurkale22, mwear, naseemkullah, obecny and OlivierAlbertini as code owners October 31, 2020 20:47

vmarchaud self-assigned this Oct 31, 2020

vmarchaud added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 31, 2020

vmarchaud force-pushed the export-result-update branch 3 times, most recently from c1d28f8 to ccdc74e Compare October 31, 2020 21:11

chore: fix tests

be5e4cf

vmarchaud force-pushed the export-result-update branch from ccdc74e to be5e4cf Compare October 31, 2020 21:35

dyladan approved these changes Nov 2, 2020

View reviewed changes

dyladan added this to In progress PRs in GA Burndown via automation Nov 2, 2020

dyladan added internal release:required-for-ga labels Nov 2, 2020

obecny approved these changes Nov 3, 2020

View reviewed changes

GA Burndown automation moved this from In progress PRs to Approved Nov 3, 2020

vmarchaud added the breaking label Nov 4, 2020

Merge branch 'master' into export-result-update

6af9a7e

dyladan merged commit 14c92da into open-telemetry:master Nov 4, 2020

GA Burndown automation moved this from Approved to Done Nov 4, 2020

vmarchaud mentioned this pull request Dec 20, 2020

metrics-exporters: uncontrolable unhandled rejection when failing to export #1498

Closed

hectorhdzg mentioned this pull request Jan 13, 2021

Updating OpenTelemetry version for Azure Exporter Azure/azure-sdk-for-js#13193

Merged

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023

chore: remove gts from packages (open-telemetry#1643)

0fbacba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: new interface for ExportResult #1569 #1643

refactor: new interface for ExportResult #1569 #1643

vmarchaud commented Oct 31, 2020

codecov bot commented Oct 31, 2020 •

edited

dyladan left a comment

dyladan Nov 2, 2020

vmarchaud Nov 2, 2020

dyladan Nov 2, 2020

vmarchaud commented Nov 2, 2020

dyladan commented Nov 2, 2020

obecny left a comment

obecny Nov 3, 2020

vmarchaud Nov 4, 2020

obecny Nov 3, 2020

vmarchaud commented Nov 4, 2020

refactor: new interface for ExportResult #1569 #1643

refactor: new interface for ExportResult #1569 #1643

Conversation

vmarchaud commented Oct 31, 2020

codecov bot commented Oct 31, 2020 • edited

Codecov Report

dyladan left a comment

Choose a reason for hiding this comment

dyladan Nov 2, 2020

Choose a reason for hiding this comment

vmarchaud Nov 2, 2020

Choose a reason for hiding this comment

dyladan Nov 2, 2020

Choose a reason for hiding this comment

vmarchaud commented Nov 2, 2020

dyladan commented Nov 2, 2020

obecny left a comment

Choose a reason for hiding this comment

obecny Nov 3, 2020

Choose a reason for hiding this comment

vmarchaud Nov 4, 2020

Choose a reason for hiding this comment

obecny Nov 3, 2020

Choose a reason for hiding this comment

vmarchaud commented Nov 4, 2020

codecov bot commented Oct 31, 2020 •

edited