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

Update enduser domain and add enduser.pseudo.id #1456

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

heyams
Copy link
Contributor

@heyams heyams commented Oct 7, 2024

Fixes #1104

@heyams heyams force-pushed the heya/add-enduser-namespace branch from 0a8b0e8 to ee0970f Compare October 7, 2024 19:06
model/enduser/registry.yaml Outdated Show resolved Hide resolved
model/enduser/registry.yaml Outdated Show resolved Hide resolved
model/enduser/registry.yaml Outdated Show resolved Hide resolved
model/enduser/registry.yaml Show resolved Hide resolved
@heyams heyams changed the title Update enduser domain and add authentication as a subdomain Update enduser domain and add enduser.authentication.id Oct 22, 2024
@heyams heyams changed the title Update enduser domain and add enduser.authentication.id Update enduser domain and add enduser.authentication.id Oct 22, 2024
@heyams
Copy link
Contributor Author

heyams commented Oct 23, 2024

need some advice with this weaver error, it didn't point to any files that I have modified. it's persisting.
image

@trask
Copy link
Member

trask commented Oct 23, 2024

need some advice with this weaver error, it didn't point to any files that I have modified. it's persisting. image

if you scroll up from there, you'll see:

ℹ Validating: $/home/weaver/target/general/attributes.md
✖ Could not find: identity

which looks related to this file that was deleted in your PR: model/enduser/deprecated/common.yaml

@heyams
Copy link
Contributor Author

heyams commented Jan 14, 2025

For people who are flip-flopping on what we discussed about sub namespace authentication, a friendly reminder to revisit these links:

#1104 (comment)
#1104 (comment)

#1172
image

or watch the SIG meeting recordings whenever i was present :)

@trask
Copy link
Member

trask commented Jan 15, 2025

For people who are flip-flopping on what we discussed about sub namespace authentication, a friendly reminder to revisit these links

hi @heyams, I don't see any arguments in those links for specifically preferring enduser.authenticated.id over enduser.id?

in order to help this PR move forward, can you write out the reasons you have for specifically preferring enduser.authenticated.id over enduser.id? (other than that you thought it was decided already)

@heyams
Copy link
Contributor Author

heyams commented Jan 15, 2025

For people who are flip-flopping on what we discussed about sub namespace authentication, a friendly reminder to revisit these links

hi @heyams, I don't see any arguments in those links for specifically preferring enduser.authenticated.id over enduser.id?

in order to help this PR move forward, can you write out the reasons you have for specifically preferring enduser.authenticated.id over enduser.id? (other than that you thought it was decided already)

it was decided in the SIG meeting, not me. recording is available though. they preferred to have user.authentication.id (not user.authenticated.id) as sub-namespace, and then there was a concern that user is too ambiguous because user needs to be used along with a parent namespace. Then we decided to bring back enduser and applied the same authentication sub-namespace. Then we had a concern that people might put authenticated user id into the enduser.id field.. that was when I was asked to come up with a list of other names. throughout this renaming discussion, nobody questioned enduser.authentication.id. now we settled with enduser.pseudo.id for enduser.id, we are back to square one, need to revisit whether or not we want enduser.authentication.

@trask
Copy link
Member

trask commented Jan 15, 2025

@heyams I still don't see any reasons in your last comment for specifically preferring enduser.authenticated.id over enduser.id? (other than that you thought it was decided already)

@trisch-me
Copy link
Contributor

hey @heyams

it might be a long thread but I think we all want to see it merged.
We have discussed this issue on semconv meeting this Monday and as @trask mentioned above (and to my personal preference) I am in favor of having just id without additional subdomain. And I don’t see clear preference for the other option.

I remember we have discussed in the beginning that it might be good to have additional sub namespace for authentication attributes and put there id and other attributes, but I think we can start simple without specific subdomain until we have a use case for it.

please let me know if it would work for you
also everyone else please chime in if you think this isn't working for some cases

@heyams
Copy link
Contributor Author

heyams commented Jan 22, 2025

hey @heyams

it might be a long thread but I think we all want to see it merged. We have discussed this issue on semconv meeting this Monday and as @trask mentioned above (and to my personal preference) I am in favor of having just id without additional subdomain. And I don’t see clear preference for the other option.

I remember we have discussed in the beginning that it might be good to have additional sub namespace for authentication attributes and put there id and other attributes, but I think we can start simple without specific subdomain until we have a use case for it.

please let me know if it would work for you also everyone else please chime in if you think this isn't working for some cases

I was planning to attend the SIG to get everyone's feedback. Monday was a public holiday here. If that was the consensus from everyone from the SIG to reuse enduser.id for the authenticated user id, I'm good with it. I can go ahead to make a change this week and then discuss it further in the next SIG if anyone else objects.

i appreciate that you reach out because this sub-namespace was your original idea.

@trisch-me
Copy link
Contributor

hey @heyams - sure, back then we wanted to find a solution on how to manage both auth and not auth id, and that was one of ideas, but now we have already user.pseudo.id, so I believe we can proceed with just user.id

@github-actions github-actions bot added the enhancement New feature or request label Jan 23, 2025
@heyams heyams changed the title Update enduser domain and add enduser.authentication.id Update enduser domain and add enduser.pseudo.id Jan 24, 2025
@heyams heyams requested review from MSNev and LikeTheSalad January 24, 2025 00:25
@trisch-me
Copy link
Contributor

@heyams there are still conflicts not resolved

Copy link
Contributor

@MSNev MSNev left a comment

Choose a reason for hiding this comment

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

Provided comments offline about possible rewording of the description for enduser.pseudo.id specifically to remove the "reveal" wording. But this would be a refinement and not a blocker.

@heyams heyams requested review from jsuereth, lmolkova and trask January 24, 2025 19:13
@heyams
Copy link
Contributor Author

heyams commented Jan 24, 2025

@joaopgrassi i don't have permission to dismiss your "request for change". Your comment no longer applied.
please review it again and unblock this PR.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

User.id for authenticated user id
8 participants