-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add singleton flags to factories and standardize attributes #12057
base: main
Are you sure you want to change the base?
Add singleton flags to factories and standardize attributes #12057
Conversation
If desired, I believe I can split this PR into several smaller PRs. However, I wanted to put this version up in order to show the overall picture. |
0717a5d
to
79e1bcb
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12057 +/- ##
==========================================
+ Coverage 91.60% 91.66% +0.05%
==========================================
Files 461 462 +1
Lines 24678 24837 +159
==========================================
+ Hits 22607 22766 +159
Misses 1689 1689
Partials 382 382 ☔ View full report in Codecov by Sentry. |
I discovered when adding tests that singleton connectors will introduce broader changes into the way we build the graph, since we're wrapping each instance as a specific type of consumer. I've not been able to get it working yet but either way, the additional complexity seems not worth for now. I believe there are some reasons why singleton connectors would be useful but I'm going to propose for now that singleton instances are supported only for receivers and exporters. In the future we can try to add singleton connectors. |
32b1ba7
to
90a4238
Compare
90a4238
to
6a38f9c
Compare
bb19bc7
to
9fc517f
Compare
This PR just cleans up some naming in the `testcomponents` package, so that no component kind has a monopoly on the generic version of a variable or function name. Subset of #12057
9fc517f
to
7772955
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.
The approach makes sense to me, just one question about naming
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.
Looks good to me.
One thing I don't like about Capabilities
is that a component developer can accidentally 'lie' about their component, which can also happen here. I don't see a great way to do this here without introducing a lot more API, so this seems reasonable enough
256d7c9
to
c498d3e
Compare
Do you have ideas for making the new attributes available to components for use in spans and metrics? (Adding the |
It is the next thing I intend to look into once this is merged. Either as you suggested, or maybe it's possible to save them into the Meter/Tracer providers in some way? |
This PR is an extension of #11344 that addresses #11814.
Major changes:
Metadata()
method toreceiver.Factory
,exporter.Factory
andconnector.Factory
(processor.Factory
excluded intentionally as described in Loggers for shared components should not report signal type. #11814). TheseMetadata()
methods returnMetadata
structs, which act similarly toconsumer.Capabilities
in that they are intended as a set of information which describes the components which the factory produces.go.opentelemetry.io/otel/attribute.Set
. This set is used as the ID in the graph, but also as the set of attributes that creates the logger. In the future, the set will be automatically used to describe other aspects of the collector's own telemetry.Minor changes:
Metadata
from the appropriate factory. There is a minor user facing change in that the error message is not wrapped in a "could not build" prefix. I moved the processors to this pattern as well in order to match the other components.