Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: zipkin-exporter: don't export after shutdown #526

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Nov 13, 2019

Which problem is this PR solving?

According to spec and exporter should return FailedNotRetryable error
after shutdown was called.

see https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/sdk-tracing.md#shutdown-1

After the call to Shutdown subsequent calls to Export are not allowed and should return FailedNotRetryable error.

Short description of the changes

Remember that shutdown() was called and if yes immediate call callback with error.

@codecov-io
Copy link

codecov-io commented Nov 13, 2019

Codecov Report

Merging #526 into master will increase coverage by 47.31%.
The diff coverage is 92.3%.

@@             Coverage Diff             @@
##           master     #526       +/-   ##
===========================================
+ Coverage    49.2%   96.52%   +47.31%     
===========================================
  Files          47      132       +85     
  Lines        1392     6361     +4969     
  Branches      143      566      +423     
===========================================
+ Hits          685     6140     +5455     
+ Misses        707      221      -486
Impacted Files Coverage Δ
.../opentelemetry-exporter-zipkin/test/zipkin.test.ts 100% <100%> (ø)
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 98.43% <85.71%> (ø)
...ckages/opentelemetry-core/src/common/NoopLogger.ts 33.33% <0%> (-16.67%) ⬇️
...metry-core/src/trace/instrumentation/BasePlugin.ts 80.55% <0%> (-5.56%) ⬇️
...ges/opentelemetry-core/src/common/ConsoleLogger.ts 93.33% <0%> (-0.79%) ⬇️
...telemetry-core/src/context/propagation/B3Format.ts 96.42% <0%> (-0.72%) ⬇️
...core/src/context/propagation/BinaryTraceContext.ts 97.5% <0%> (-0.54%) ⬇️
...core/src/context/propagation/NoopHttpTextFormat.ts 100% <0%> (ø) ⬆️
.../opentelemetry-core/src/trace/spancontext-utils.ts 100% <0%> (ø) ⬆️
...ages/opentelemetry-core/src/internal/validators.ts 100% <0%> (ø) ⬆️
... and 125 more

According to spec and exporter should return FailedNotRetryable error
after shutdown was called.
Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like same thing needs to be implemented for Jaeger exporter.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, one comment

packages/opentelemetry-exporter-zipkin/src/zipkin.ts Outdated Show resolved Hide resolved
@Flarna
Copy link
Member Author

Flarna commented Nov 15, 2019

Seems we have an issue with the just released node type definitions (DefinitelyTyped/DefinitelyTyped#40118):

src/http.ts:343:15 - error TS2345: Argument of type 'UrlWithStringQuery | null' is not assignable to parameter of type 'RequestOptions | (RequestOptions & Partial<UrlWithParsedQuery>) | null'.
  Type 'UrlWithStringQuery' is not assignable to type 'RequestOptions | (RequestOptions & Partial<UrlWithParsedQuery>) | null'.
    Type 'UrlWithStringQuery' is not assignable to type 'RequestOptions & Partial<UrlWithParsedQuery>'.
      Type 'UrlWithStringQuery' is not assignable to type 'RequestOptions'.
        Types of property 'protocol' are incompatible.
          Type 'string | null' is not assignable to type 'string | undefined'.
            Type 'null' is not assignable to type 'string | undefined'.

@OlivierAlbertini OlivierAlbertini added the bug Something isn't working label Nov 18, 2019
@mayurkale22 mayurkale22 merged commit 41fd196 into open-telemetry:master Nov 19, 2019
@Flarna Flarna deleted the fix_zipkin-export_shutdown branch November 19, 2019 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants