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

chore: v1.0.0 proposal #2468

Merged
merged 2 commits into from
Sep 30, 2021
Merged

chore: v1.0.0 proposal #2468

merged 2 commits into from
Sep 30, 2021

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Sep 13, 2021

There are no changes from 0.26.0

Huge thank you to all involved who helped get us this far! 🎉

@dyladan dyladan requested a review from a team September 13, 2021 13:04
@codecov
Copy link

codecov bot commented Sep 13, 2021

Codecov Report

Merging #2468 (c28965a) into main (6afe2fc) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2468   +/-   ##
=======================================
  Coverage   93.23%   93.23%           
=======================================
  Files         137      137           
  Lines        5042     5042           
  Branches     1066     1066           
=======================================
  Hits         4701     4701           
  Misses        341      341           

@vmarchaud
Copy link
Member

The only major caveat I see is that the collector exporter depends on the experimental metrics package, but that is probably ok.

I'm fine with it, at the end of the line that means we'll have to bump major when the metrics api will be merged with the API pkg.

Also there were discussing about renaming the exporter collector, do we want to do this before 1.0 ? #2364

@dyladan
Copy link
Member Author

dyladan commented Sep 13, 2021

@open-telemetry/javascript-maintainers @open-telemetry/javascript-approvers I'm thinking we should probably hold back sdk-node as experimental until we can remove its dependence upon experimental and contrib packages. WDYT?

@willarmiros
Copy link
Contributor

@open-telemetry/javascript-maintainers @open-telemetry/javascript-approvers I'm thinking we should probably hold back sdk-node as experimental until we can remove its dependence upon experimental and contrib packages. WDYT?

Just to confirm, @opentelemetry/sdk-trace-node would still be 1.0? If so that makes sense to me.

@dyladan
Copy link
Member Author

dyladan commented Sep 13, 2021

@open-telemetry/javascript-maintainers @open-telemetry/javascript-approvers I'm thinking we should probably hold back sdk-node as experimental until we can remove its dependence upon experimental and contrib packages. WDYT?

Just to confirm, @opentelemetry/sdk-trace-node would still be 1.0? If so that makes sense to me.

yes

@dyladan
Copy link
Member Author

dyladan commented Sep 14, 2021

Updated to put sdk-node in experimental which will give us time to asses its dependencies and public interface.

@vmarchaud
Copy link
Member

What do we do about #2364 ? That would be preferable to do it now before 1.0 wdyt ?

@dyladan
Copy link
Member Author

dyladan commented Sep 15, 2021

What do we do about #2364 ? That would be preferable to do it now before 1.0 wdyt ?

Created a PR #2476

Copy link
Contributor

@willarmiros willarmiros left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

finally 🚀

@obecny
Copy link
Member

obecny commented Sep 16, 2021

exporter collector can exports traces and metrics, metrics are experimental, not sure though what we should be doing with this than, Should we leave it, move exporters to experimental or split it etc. or something else ?

@willarmiros
Copy link
Contributor

@obecny yeah that was the other issue that Dan called out in the description. Some SIGs like Python and .NET have went ahead and marked their exporters as stable.

But we could move them into separate packages with traces in stable and metrics in experimental if we wanted to avoid possible major version bumps. Do you know how much work that would be?

@legendecas
Copy link
Member

As @opentelemetry/api-metrics is not stable yet, I'm wondering how would it be meaningful to mark @opentelemetry/exporter-prometheus as stable? It's not like @opentelemetry/exporter-collector-* that export traces, exporter-prometheus only export metrics.

@obecny
Copy link
Member

obecny commented Sep 17, 2021

@obecny yeah that was the other issue that Dan called out in the description. Some SIGs like Python and .NET have went ahead and marked their exporters as stable.

But we could move them into separate packages with traces in stable and metrics in experimental if we wanted to avoid possible major version bumps. Do you know how much work that would be?

quite a lot of work tbh

@aabmass
Copy link
Member

aabmass commented Sep 17, 2021

@opentelemetry/exporter-collector-* depends on @opentelemetry/sdk-metric-base

  • This is probably OK, but if the metric export interface changes it will require a major version bump of the exporter

In Python, we removed the OTLP metrics exporter altogether. The metrics spec has changed so much we decided we would need to reimplement it anyway.

However, since JS has separated the metrics and trace api/sdk altogether, it seems reasonable to separate the trace and metric OTLP exporters.

@willarmiros
Copy link
Contributor

As @opentelemetry/api-metrics is not stable yet, I'm wondering how would it be meaningful to mark @opentelemetry/exporter-prometheus as stable? It's not like @opentelemetry/exporter-collector-* that export traces, exporter-prometheus only export metrics.

I agree, Prometheus exporter should stay in experimental.

quite a lot of work tbh

So it seems like this would require splitting the existing 3 OTLP exporters (@opentelemetry/exporter-otlp-http, @opentelemetry/exporter-otlp-grpc, and @opentelemetry/exporter-otlp-proto) into 9 distinct packages if we wanted to do this correctly/in line with the pattern of other components:

  1. Move the OTLPTraceExporter classes (and transform file for HTTP exporter) into new packages called @opentelemetry/exporter-trace-otlp-*. These packages will be marked as stable.
  2. Move the OTLPMetricExporter classes (and transformMetrics file for HTTP exporter) into new packages called @opentelemetry/exporter-metric-otlp-*. These packages will be marked as experimental.
  3. Remove dependencies on metrics & traces SDKs/APIs from original exporter packages. They can now be released as stable.
  4. (optional) rename the existing exporter packages, to @opentelemetry/exporter-base-otlp-* (or leave as is for simplicity).

@open-telemetry/javascript-approvers What do you think of the approach and package naming in this proposal? If we're agreed this is something we should do before 1.0.0 I feel like this would be only slightly more work than #2476.

@legendecas
Copy link
Member

legendecas commented Sep 17, 2021

I would vote for separating tracing and metrics collection for OTLP exporter and put them on different stage as metrics spec has changed so much since then.

@obecny
Copy link
Member

obecny commented Sep 17, 2021

As @opentelemetry/api-metrics is not stable yet, I'm wondering how would it be meaningful to mark @opentelemetry/exporter-prometheus as stable? It's not like @opentelemetry/exporter-collector-* that export traces, exporter-prometheus only export metrics.

I agree, Prometheus exporter should stay in experimental.

quite a lot of work tbh

So it seems like this would require splitting the existing 3 OTLP exporters (@opentelemetry/exporter-otlp-http, @opentelemetry/exporter-otlp-grpc, and @opentelemetry/exporter-otlp-proto) into 9 distinct packages if we wanted to do this correctly/in line with the pattern of other components:

  1. Move the OTLPTraceExporter classes (and transform file for HTTP exporter) into new packages called @opentelemetry/exporter-trace-otlp-*. These packages will be marked as stable.
  2. Move the OTLPMetricExporter classes (and transformMetrics file for HTTP exporter) into new packages called @opentelemetry/exporter-metric-otlp-*. These packages will be marked as experimental.
  3. Remove dependencies on metrics & traces SDKs/APIs from original exporter packages. They can now be released as stable.
  4. (optional) rename the existing exporter packages, to @opentelemetry/exporter-base-otlp-* (or leave as is for simplicity).

@open-telemetry/javascript-approvers What do you think of the approach and package naming in this proposal? If we're agreed this is something we should do before 1.0.0 I feel like this would be only slightly more work than #2476.

how did you count 9 ? it would be 6 exporters, metrics inherits from tracing this is how it is built the difference is in transport layer and data conversion

@willarmiros
Copy link
Contributor

how did you count 9 ? it would be 6 exporters, metrics inherits from tracing this is how it is built the difference is in transport layer and data conversion

Ah yes that makes sense. I thought for each protocol we would have a "base" exporter, then trace-specific logic would be in a separate package, and metric-specific logic in yet another. So 3 packages * 3 protocols = 9.

But if we're leaving the trace logic in the existing exporter and just creating a new package for metric exporters, then that's easier and simpler. We can leave the exporter package names as-is. Do you agree with the naming of @opentelemetry/exporter-metric-otlp-* for the new ones?

I unfortunately don't have time to work on this today, but if anyone else could take it up that'd be awesome. Otherwise maybe myself or @dyladan could have time next week.

@vmarchaud vmarchaud self-requested a review September 18, 2021 19:39
@vmarchaud
Copy link
Member

I would like to add that when logs spec will be done, it might be easier to have them as a seperate experimental exporter. Could we maybe let the exporter still be experimental for now while we make the effort to split metrics out ? So we can still GA SDKs (which i believe is the main goal here)

@dyladan
Copy link
Member Author

dyladan commented Sep 20, 2021

I like the idea of having a separate exporter package for each signal. Ideally I think the metrics would not depend on the trace exporter, but that's an internal change that can be made after 1.0

@dyladan
Copy link
Member Author

dyladan commented Sep 20, 2021

@aabmass are you volunteering to take on that project?

@willarmiros
Copy link
Contributor

@dyladan I'm not sure if you meant to tag me there but yes I can take a stab at this later today. I'll take the approach with 6 total packages (where the metric exporters depend on the trace exporters) unless anyone thinks otherwise.

@dyladan
Copy link
Member Author

dyladan commented Sep 20, 2021

I'm not sure what made me tag Aaron oops. I probably meant to tag you :)

@dyladan
Copy link
Member Author

dyladan commented Sep 20, 2021

I'll take the approach with 6 total packages (where the metric exporters depend on the trace exporters) unless anyone thinks otherwise.

This seems reasonable. As a follow-up we can factor out the shared parts into a base package if that makes sense later. It can be done after 1.0 since I wouldn't expect that to be a breaking change.

@obecny
Copy link
Member

obecny commented Sep 21, 2021

I will refactor this, but I think we should move otlp exporters to experimental package first

@willarmiros
Copy link
Contributor

Sorry all, still working on this. I will have something out by later tomorrow. The tricky bit is that due to the repo splitting, we cannot depend directly on the trace/base exporters from the metric exporters after they're moved to experimental. So I've done pretty much all of the work of separating them, but now I still need to verify the metric exporters will work without having to make any breaking changes to the base exporter after releasing it.

@rauno56
Copy link
Member

rauno56 commented Sep 21, 2021

Do you think we could release this as 0.x instead for now?
It would be of great value to get the changes listed here public and make the actual 1.0 release "less grand".

@willarmiros
Copy link
Contributor

Went ahead and opened #2486 to continue discussion on separating exporters. We can leave this PR to discuss 1.0.0 release.

@willarmiros
Copy link
Contributor

Per discussion in SIG this is now only blocked on #2490.

@dyladan
Copy link
Member Author

dyladan commented Sep 30, 2021

Updated and ready for ✔️

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

🎊

Copy link
Member

@johnbley johnbley left a comment

Choose a reason for hiding this comment

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

🎉

@dyladan dyladan merged commit 69b925d into open-telemetry:main Sep 30, 2021
@dyladan dyladan deleted the proposal-1.0 branch September 30, 2021 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.