Skip to content

Move capturing enduser.id attribute behind a flag#9751

Merged
laurit merged 3 commits into
open-telemetry:mainfrom
laurit:enuser-id-flag
Oct 31, 2023
Merged

Move capturing enduser.id attribute behind a flag#9751
laurit merged 3 commits into
open-telemetry:mainfrom
laurit:enuser-id-flag

Conversation

@laurit

@laurit laurit commented Oct 24, 2023

Copy link
Copy Markdown
Contributor

Resolves #9740

@laurit laurit requested a review from a team October 24, 2023 09:13
Comment thread instrumentation/servlet/README.md Outdated
| ---------------------------------------------------------------------- | ------- | ------- | --------------------------------------------------- |
|------------------------------------------------------------------------| ------- | ------- |-----------------------------------------------------|
| `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
Copy Markdown
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
Copy Markdown
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?

@trask trask Oct 24, 2023

Copy link
Copy Markdown
Member

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

philsttr commented Oct 31, 2023

Copy link
Copy Markdown
Contributor

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

laurit commented Oct 31, 2023

Copy link
Copy Markdown
Contributor Author

@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

philsttr commented Oct 31, 2023

Copy link
Copy Markdown
Contributor

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
@laurit

laurit commented Oct 31, 2023

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Contributor

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

@trask

trask commented Oct 31, 2023

Copy link
Copy Markdown
Member

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

@philsttr

Copy link
Copy Markdown
Contributor

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

@philsttr

Copy link
Copy Markdown
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