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

Move capturing enduser.id attribute behind a flag #9751

Merged
merged 3 commits into from
Oct 31, 2023

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Oct 24, 2023

Resolves #9740

@laurit laurit requested a review from a team October 24, 2023 09:13
| `otel.instrumentation.servlet.experimental-span-attributes` | Boolean | `false` | Enable the capture of experimental span attributes. |
| `otel.instrumentation.servlet.experimental.enduser-span-attributes` | Boolean | `false` | Enable the capture of `enduser.id` span attribute. |
Copy link
Member

Choose a reason for hiding this comment

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

do you think it makes sense to generalize this from .servlet. to .common. in anticipation of #9400?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, but was unclear where the common flag could be documented (I guess it is in the website repo). So just added it for the servlet now. What do you suggest for the flag name? Is otel.instrumentation.common.capture-enduser.enabled that was suggested in #9400 fine?

Copy link
Member

@trask trask Oct 24, 2023

Choose a reason for hiding this comment

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

Is otel.instrumentation.common.capture-enduser.enabled that was suggested in #9400 fine?

👍

@philsttr
Copy link
Contributor

philsttr commented Oct 31, 2023

Heads up, #9777 also defines a common flag for capturing enduser.* attributes. In addition, it defines flags for enabling/disabling specific enduser attributes.

It would be great to resolve the differences between these two PRs.

(Sorry, I just noticed this PR today)

@laurit
Copy link
Contributor Author

laurit commented Oct 31, 2023

@philsttr I'll just merge this and leave resolving the differences to you :) I'll also create an issue for documenting this flag(or flags) that are added here or in your pr in the website repo

@philsttr
Copy link
Contributor

philsttr commented Oct 31, 2023

Ok, I used otel.instrumentation.common.enduser.enabled for the name of the common property. Can you change this PR to use that before merging? so at least the other PR won't rename any properties, even if it changes the implementation a bit.

@laurit laurit merged commit 05ae636 into open-telemetry:main Oct 31, 2023
46 checks passed
@laurit
Copy link
Contributor Author

laurit commented Oct 31, 2023

@philsttr I merged this pr as is, you are welcome to rename the added property in your pr.

@laurit laurit deleted the enuser-id-flag branch October 31, 2023 13:29
@philsttr
Copy link
Contributor

Ok, I just didn't want to introduce a backwards incompatibility in case there is a release before my PR is merged.

@trask
Copy link
Member

trask commented Oct 31, 2023

@philsttr if you don't mind sending a small PR to do the rename we can merge that quickly

@philsttr
Copy link
Contributor

@trask can I get your opinion on #9777 (comment) to finalize the name/flags?

@philsttr
Copy link
Contributor

@philsttr if you don't mind sending a small PR to do the rename we can merge that quickly

@trask Done. See #9788

Abhishekkr3003 pushed a commit to Abhishekkr3003/opentelemetry-java-instrumentation that referenced this pull request Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enduser.id should not be captured by default
4 participants