-
Notifications
You must be signed in to change notification settings - Fork 358
[Exporter.Geneva] Opt in resources #3552
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
base: main
Are you sure you want to change the base?
Conversation
| /// WantedResourceAttributes specifies which resource attribute fields should be sent to Geneva. | ||
| /// | ||
| /// Any resource attributes not in WantedResourceAttributes are ignored. | ||
| /// If WantedResourceAttributes is not provided, all resource attributes will be sent to Geneva. |
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.
all will be sent or none will be sent?
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.
In my opinion, it should be none, so the behavior is back-compatible. To get resource attributes, users must explicitly opt-in.
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.
if a customer really wants all resource attributes, how might they accomplish that? By the way, there is a feature flag which is default off that controls this feature
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've made the change you requested
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.
do you think the special resource attributes, service.name and service.instanceId, which are mapped to part A cloud extension, should also be affected by WithResourceAttributes being set to null?
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.
By the way, there is a feature flag which is default off that controls this feature
I am not sure if we need to feature flag this at all. The WithResourceAttribute is sufficient on its own right?
if a customer really wants all resource attributes, how might they accomplish that?
By listing each attribute. Not ideal, but I also don't think this will be the common case with GenevaExporter. If there is strong need for such a capability, we can introduce a boolean 'with_all_resource`/similar that adds all attributes effectively ignore WithResourceAttributes. Alternatively, "*" can be used to mean - get all attributes - we do such thing elsewhere in GenevaExporter.
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 can remove the feature flag in a future PR
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.
feature flag has been removed, and the behaviour of the attribute is that no resource attributes are sent if it is null
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| } | ||
| } | ||
|
|
||
| if (!isDedicatedField && (this.WithResourceAttributes == null || !this.WithResourceAttributes.Contains(entry.Key))) |
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.
shouldn't we short-circuit this entire method if this.WithResourceAttributes == null ?
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 decided that service.name and service.instanceId fields should still be used even if they're not explicitly mentioned in WithResourceAttributes, because they have a special mapping into part A cloud extension. So I don't believe the code should skip this section if it is one of those two resource attributes
| /// Any resource attributes not in WithResourceAttributes are ignored. | ||
| /// If WithResourceAttributes is not provided, no resource attributes will be sent to Geneva. | ||
| /// </summary> | ||
| public IEnumerable<string>? WithResourceAttributes { get; set; } |
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.
nit: is this still controlled by a feature flag in connection string?
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.
please refer to this previous conversation: #3552 (comment)
| /// Any resource attributes not in WithResourceAttributes are ignored. | ||
| /// If WithResourceAttributes is not provided, no resource attributes will be sent to Geneva. | ||
| /// </summary> | ||
| public IEnumerable<string>? WithResourceAttributes { get; set; } |
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.
Should this be --> public IEnumerable? WithResourceAttributes { get; init; }
Also, AllowedResourceAttributes or ResourceAttributeFilter naming might fit better here. No strong preference will leave it to you.
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.
sure - would you like me to do the same to PrepopulatedFields?
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
90ce62b to
5385b5e
Compare
src/OpenTelemetry.Exporter.Geneva/.publicApi/PublicAPI.Shipped.txt
Outdated
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MsgPackTraceExporter.cs
Show resolved
Hide resolved
src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MsgPackTraceExporter.cs
Show resolved
Hide resolved
|
Consider expanding the test coverage for these scenarios
|
5255e5d to
9ac9337
Compare
Except for service.name mapping, which was already tested, I've added all of these tests in the new revision |
This is a re-submission of #3367 , after @cijothomas recommend that it should be re-introduced.
I did change the name of the config option to WithResourceAttributes to match the Rust option in the user events log exporter
I also changed the logic when this field is not provided. The default is now to include no resource attributes except for mapped fields (eg., service.name -> Part A cloud extension)
Changes
Allows users of the Geneva exporter to specify which resource attributes they want sent to Geneva. This prevents excessive trace data from being exported if there are many resource attributes which do not need to be stored.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes