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

[Exporter.OTLP] Scaffolding for separate projects #4279

Closed
wants to merge 6 commits into from

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Mar 7, 2023

Towards #3421 (comment)

Changes

Scaffolding for OTLP over HTTP and gRPC separate projects.
gRPC project was in place.

PR should move forward with support for OTLP environmental variables specific for each signal. I do not see any possibility to do it without introducing breaking changes to current, unified exporter. There were a proposal to split it into GRPC and HTTP. It allows us to introduce breaking changes, see comment from @CodeBlanch: #4154 (comment).

Second benefit - possibility to use only HTTP version without bringing dependencies to gRPC client. Based on #3421 (comment) - it can reduce instrumented project by 17MB. It can be significant difference if you are using a lot of different micro-services.

As a results we will have two projects:

  • OpenTelemetry.Exporter.OpenTelemetryProtocol.Grpc - handling grpc protocol
  • OpenTelemetry.Exporter.OpenTelemetryProtocol.Http - nandling http/protobuf

For significant contributions please make sure you have completed the following items:

  • [ ] Appropriate CHANGELOG.md updated for non-trivial changes
  • [ ] Design discussion issue #
  • [ ] Changes in public API reviewed

@Kielek Kielek requested a review from a team March 7, 2023 13:02
@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #4279 (60dbb80) into main (f3dab75) will decrease coverage by 0.07%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4279      +/-   ##
==========================================
- Coverage   84.31%   84.25%   -0.07%     
==========================================
  Files         296      296              
  Lines       11791    11791              
==========================================
- Hits         9942     9934       -8     
- Misses       1849     1857       +8     
Impacted Files Coverage Δ
...tation/OpenTelemetryProtocolExporterEventSource.cs 85.00% <0.00%> (-15.00%) ⬇️
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs 72.72% <0.00%> (-13.64%) ⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 97.14% <0.00%> (-2.86%) ⬇️
...tpListener/Internal/PrometheusCollectionManager.cs 73.62% <0.00%> (-2.20%) ⬇️
...lementation/SqlClientInstrumentationEventSource.cs 75.00% <0.00%> (+4.16%) ⬆️

@reyang
Copy link
Member

reyang commented Mar 7, 2023

@Kielek would you put a brief summary regarding why would we want to have two separate projects/components for OTLP Exporter?

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Need to understand the direction #4279 (comment).

@pellared
Copy link
Member

pellared commented Mar 7, 2023

@reyang #3885 (comment)

@reyang
Copy link
Member

reyang commented Mar 7, 2023

@reyang #3885 (comment)

I looked at the comments and didn't get the clarity. May I get a summary of what does it mean? - e.g. what exact problem do we want to solve

@Kielek
Copy link
Contributor Author

Kielek commented Mar 8, 2023

@reyang, I am trying to move forward with support for OTLP environmental variables specific for each signal. I do not see any possibility to do it without introducing breaking changes to current, unified exporter. There were a proposal to split it into GRPC and HTTP. It allows us to introduce breaking changes, see comment from @CodeBlanch: #4154 (comment).

There is also a second benefit - possibility to use only HTTP version without bringing dependencies to gRPC client. Based on #3421 (comment) - it can reduce instrumented project by 17MB. It can be significant difference if you are using a lot of different micro-services.

@Kielek Kielek requested a review from reyang March 8, 2023 06:09
@reyang
Copy link
Member

reyang commented Mar 8, 2023

My main concern about this PR - it seems we make random moves without clear direction/picture. E.g. What 's our vision? What do we plan to do with https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/file-exporter.md and other potential upcoming "flavors of OTLP exporters"?

@reyang, I am trying to move forward with support for OTLP environmental variables specific for each signal. I do not see any possibility to do it without introducing breaking changes to current, unified exporter. There were a proposal to split it into GRPC and HTTP. It allows us to introduce breaking changes, see comment from @CodeBlanch: #4154 (comment).

I don't think "introducing environment variable" could justify "changing how components are layered/organized".

There is also a second benefit - possibility to use only HTTP version without bringing dependencies to gRPC client. Based on #3421 (comment) - it can reduce instrumented project by 17MB. It can be significant difference if you are using a lot of different micro-services.

I would say the right direction is to work with the gRPC/protobuf owner and see how to implement OTLP/gRPC exporter without having to take 17MB extra packages.

@reyang
Copy link
Member

reyang commented Mar 8, 2023

@Kielek would you update the PR description to make sure the overall thinking is captured? Here goes a good example from @Yun-Ting's PR #3430.

@Kielek
Copy link
Contributor Author

Kielek commented Mar 8, 2023

@Kielek would you update the PR description to make sure the overall thinking is captured? Here goes a good example from @Yun-Ting's PR #3430.

Description updated.

@alanwest
Copy link
Member

alanwest commented Mar 9, 2023

My main concern about this PR - it seems we make random moves without clear direction/picture.

I agree we need to consider our direction a bit more deeply prior to continuing work to create separate projects. We have discussed various things we could do, but have not yet devoted the focus necessary to settle on what to actually do.

There are a number of different issues, some of which may be easier to address with separate packages, but I'm not yet convinced it's our only option.

  1. Support for signal-specific environment variables.

    • Even if we did ultimately create separate packages, I'd ideally still want to try supporting signal-specific environment variables with the original package. I think it would be odd to tell users they have to use one of the new packages to get this functionality. Though, to be honest I have thought much about how we'd make this work.
  2. Eliminating the use of the very soon to be deprecated gRPC library.

  3. Better support for DI/configuration integration (the single OTLP exporter options class causes us some grief here).

@reyang
Copy link
Member

reyang commented Mar 9, 2023

  1. Eliminating the use of the very soon to be deprecated gRPC library.

This sounds like music 🎵 to me 👍
We should seek some input from @JamesNK.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 16, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 24, 2023
@Kielek Kielek deleted the split-otlp/part-1 branch January 25, 2024 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants