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

feat(sdk-trace)!: remove ability to have BasicTracerProvider instantiate exporters #5239

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Dec 6, 2024

Which problem is this PR solving?

This is apparently a feature we offer - overriding a static property to register exporters, which can then be instantiated via env vars. And also overriding methods that facilitate the creation of exporter via env vars. It ends up in both Web and Node.js tracing packages.

I don't recall the context of why this was added but I know it was used in NodeSDK but that offers a better approach now that does not rely on that behavior anymore. I reckon few of our users even know that this feature exists.

This PR proposes removing this feature. If this feature is needed, we can just provide a function that does this (or they can implement it themselves, it's trivial to do), and require the person to pass it the result to the constructor as spanProcessors, which should be much cleaner.

I'm not sure yet what to do with propagators which offer a similar functionality, but I'm confident that we can come up with a simpler to use/maintain/understand solution for that.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Unit tests

@pichlermarc pichlermarc added target:next-major-release This PR targets the next major release (`next` branch) pkg:sdk-trace-base pkg:sdk-trace-node labels Dec 6, 2024
@pichlermarc pichlermarc changed the title feat(sdk-trace): remove ability to statically register exporters to BasicTracerProvider class feat(sdk-trace)!: remove ability to statically register exporters to BasicTracerProvider class Dec 6, 2024
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.52%. Comparing base (47212bb) to head (2eae41a).
Report is 1 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #5239      +/-   ##
==========================================
- Coverage   94.56%   94.52%   -0.04%     
==========================================
  Files         316      316              
  Lines        8037     8020      -17     
  Branches     1628     1622       -6     
==========================================
- Hits         7600     7581      -19     
- Misses        437      439       +2     
Files with missing lines Coverage Δ
...elemetry-sdk-trace-base/src/BasicTracerProvider.ts 95.55% <ø> (-0.71%) ⬇️

... and 1 file with indirect coverage changes

@pichlermarc pichlermarc changed the title feat(sdk-trace)!: remove ability to statically register exporters to BasicTracerProvider class feat(sdk-trace)!: remove ability to have BasicTracerProvider instantiate exporters Dec 6, 2024
@pichlermarc pichlermarc marked this pull request as ready for review December 6, 2024 17:51
@pichlermarc pichlermarc requested a review from a team as a code owner December 6, 2024 17:51
@@ -255,16 +235,4 @@ export class BasicTracerProvider implements TracerProvider {
});
}
}

protected _buildExporterFromEnv(): SpanExporter | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

so IIUC it means now BasicTraceProvider wont' look up to env to resolve the exporter and it is mandatory to pass it into the constructor right?

This means NodeSDK or any other component creating the provider is responsible to resolve the exporter from env if applies right?

Copy link
Member Author

@pichlermarc pichlermarc Dec 9, 2024

Choose a reason for hiding this comment

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

so IIUC it means now BasicTraceProvider wont' look up to env to resolve the exporter and it is mandatory to pass it into the constructor right?

This means NodeSDK or any other component creating the provider is responsible to resolve the exporter from env if applies right?

Yep - that's the solution I'd propose. 🙂

This PR is just a proposal, so we don't have to do it, but I feel like this is a cleaner approach as we'd eliminate an additional (confusing) way to accomplish the same thing.

Neither the LoggerProvider nor MeterProvider have equivalent functionality and I'm not aware of any requests that I remember that asked for adding it. 🤔 Removing it also has the bonus of aligning functionality between the three signals. I'd like to align the three signals as much as possible to give a more consistent experience to end-users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:sdk-trace-base pkg:sdk-trace-node target:next-major-release This PR targets the next major release (`next` branch)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants