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(deps): use 1.x trace otlp http exporter #2788

Merged
merged 7 commits into from
Feb 24, 2022

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Feb 15, 2022

Some packages incorrectly depend on the unstable otlp http trace exporter

@dyladan dyladan added bug Something isn't working dependencies Pull requests that update a dependency file labels Feb 15, 2022
@dyladan dyladan requested a review from a team as a code owner February 15, 2022 19:10
@Flarna
Copy link
Member

Flarna commented Feb 15, 2022

Is this maybe the reason for the problems we saw after updating SDK to use API 1.1? see #2737 (comment)?

@dyladan
Copy link
Member Author

dyladan commented Feb 15, 2022

Yes it is likely to be related

@rauno56
Copy link
Member

rauno56 commented Feb 16, 2022

Unless we publish those first, everything brakes outside of the lerna-linked repo. Current latest for @opentelemetry/exporter-trace-otlp-http is 0.27.0.

@dyladan
Copy link
Member Author

dyladan commented Feb 17, 2022

Unless we publish those first, everything brakes outside of the lerna-linked repo. Current latest for @opentelemetry/exporter-trace-otlp-http is 0.27.0.

So i guess i'll hold off on the examples for now but still update the linked packages?

@rauno56
Copy link
Member

rauno56 commented Feb 21, 2022

There's no reason I know of to hold off publishing the exporter 1.0's. I think that should be done asap.

Even if we did change only the linked packages the npm installs would still break after we publish the packages that depend on unpublished exporter versions.

@codecov
Copy link

codecov bot commented Feb 22, 2022

Codecov Report

Merging #2788 (a3347ef) into main (b8df001) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2788   +/-   ##
=======================================
  Coverage   93.41%   93.41%           
=======================================
  Files         159      159           
  Lines        5450     5450           
  Branches     1145     1145           
=======================================
  Hits         5091     5091           
  Misses        359      359           
Impacted Files Coverage Δ
...s/exporter-trace-otlp-http/src/OTLPExporterBase.ts 100.00% <ø> (ø)

@dyladan
Copy link
Member Author

dyladan commented Feb 22, 2022

There's no reason I know of to hold off publishing the exporter 1.0's. I think that should be done asap.

Even if we did change only the linked packages the npm installs would still break after we publish the packages that depend on unpublished exporter versions.

Ah yes they should have been released immediately after #2626. Since their versions are already bumped, this just requires a manual publish step. I'll create an issue for this to get maintainer thumbs up because no PR will be needed.

@dyladan
Copy link
Member Author

dyladan commented Feb 22, 2022

There's no reason I know of to hold off publishing the exporter 1.0's. I think that should be done asap.
Even if we did change only the linked packages the npm installs would still break after we publish the packages that depend on unpublished exporter versions.

Ah yes they should have been released immediately after #2626. Since their versions are already bumped, this just requires a manual publish step. I'll create an issue for this to get maintainer thumbs up because no PR will be needed.

This PR still should merge before we release them because if not we will have released OTLP exporters depending on the old 0.27 versions in production which is not good.

@dyladan
Copy link
Member Author

dyladan commented Feb 22, 2022

@rauno56 actually its worse than I thought here. Because the 1.0 of the exporters (unreleased) introduces a breaking change, the exporters that depend on them are failing to build here when they are linked back together by lerna. The only reason they build successfully now is that they depend on outdated versions which have not removed the breaking property _isShutdown.

We have 2 options

  1. Bring back _isShutdown before releasing the exporters
  2. Update the dependent exporters to not use _isShutdown property

@dyladan
Copy link
Member Author

dyladan commented Feb 22, 2022

/cc @legendecas @rauno56 @vmarchaud for the above comment

@Flarna
Copy link
Member

Flarna commented Feb 23, 2022

I vote for (2).
Reverting an useful change sounds strange to me. Wasn't the 0.x/experimental phase invented for such cases?

@legendecas
Copy link
Member

I'd find (2) would be more intuitive to OTLPExporterBase implementations. TBH, prevent from sending new data on export when already shutdown should be handled well in OTLPExporterBase already and we don't have to repeat everywhere for it.

@vmarchaud
Copy link
Member

I agree for 2) too, sorry for not seeing this before :/

@dyladan
Copy link
Member Author

dyladan commented Feb 24, 2022

Lint failure is caused by an error with the badge links again

@legendecas
Copy link
Member

@dyladan it seems like it is a network jittering. Retried and it is passing.

@dyladan dyladan merged commit def9ba3 into open-telemetry:main Feb 24, 2022
@dyladan dyladan deleted the trace-exporter-version-fix branch February 24, 2022 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants