-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[service/telemetry] Switch to a factory pattern #10001
Conversation
d9e2cdf
to
c5768de
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10001 +/- ##
==========================================
- Coverage 91.65% 91.30% -0.35%
==========================================
Files 356 364 +8
Lines 16843 16745 -98
==========================================
- Hits 15437 15289 -148
- Misses 1063 1117 +54
+ Partials 343 339 -4 ☔ View full report in Codecov by Sentry. |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
6bd178a
to
01d4ff1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Feels like a nice clean up.
|
||
// Fetch data for internal telemetry like instance id and sdk version to provide for internal telemetry. | ||
res := resource.New(set.BuildInfo, cfg.Telemetry.Resource) | ||
pcommonRes := pdataFromSdk(res) | ||
|
||
logger := tel.Logger() | ||
telFactory := telemetry.NewFactory() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To highlight the point above, I don't like making 2 calls to NewFactory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach. It addresses #4970 in a way that feels consistent with the rest of our APIs.
// use the NewFactory to implement it. | ||
type Factory interface { | ||
// CreateDefaultConfig creates the default configuration for the telemetry. | ||
// TODO: Should we just inherit from component.Factory? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
component.Factory
would feel a little more uniform, though it's not clear to me how much a telemetry Factory really resembles a component factory. In particular, components have types, as where I don't know whether a telemetry factory will have a type.
- Will we want to utilize multiple telemetry factories within a service?
- Do we think we may want to offer alternate factories and let users choose between them in the service config?
I think what's here is alright for now, I just wanted to start the discussion around this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd prefer not to depend on anything from component
in here, since this struct this factory builds is not a component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed on not having a dependency on component
, I think we'd want to consider what commonalities the two have and move all of those things into a shared package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we want to utilize multiple telemetry factories within a service?
I feel like the answer to that is no (but API users are still free to do that by building their own factory that glues the two together). The type could be useful for having a consistent way to refer to what telemetry factory you used in logs/telemetry.
Do we think we may want to offer alternate factories and let users choose between them in the service config?
Probably also no? You should be able to eventually configure this through the builder IMO, but after that the service::telemetry
config section should only support the config from your telemetry factory.
I believe all remaining discussions are things I can solve while I continue working on #4970, so I will merge this right after we release v0.101.0 |
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Switches `service/telemetry` to a factory pattern. To avoid adding a lot of public API in one go: 1. the actual factory builder is in an internal package 2. I have not added the `CreateMeterProvider` method yet There are two goals with this: one is to make progress on open-telemetry#4970, the other is to allow initializing telemetry sooner: <!-- Issue number if applicable --> #### Link to tracking issue Updates open-telemetry#4970. <!--Describe what testing was performed and which tests were added.--> #### Testing Updates existing tests to use `NewFactory`
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Switches `service/telemetry` to a factory pattern. To avoid adding a lot of public API in one go: 1. the actual factory builder is in an internal package 2. I have not added the `CreateMeterProvider` method yet There are two goals with this: one is to make progress on open-telemetry#4970, the other is to allow initializing telemetry sooner: <!-- Issue number if applicable --> #### Link to tracking issue Updates open-telemetry#4970. <!--Describe what testing was performed and which tests were added.--> #### Testing Updates existing tests to use `NewFactory`
Description
Switches
service/telemetry
to a factory pattern. To avoid adding a lot of public API in one go:CreateMeterProvider
method yetThere are two goals with this: one is to make progress on #4970, the other is to allow initializing telemetry sooner:
Link to tracking issue
Updates #4970.
Testing
Updates existing tests to use
NewFactory