Add authentication sub-namespace to user #1146

heyams · 2024-06-11T23:31:12Z

I chose user.anonymous_id because of #1121

model/registry/user.yaml

thompson-tomo · 2024-06-13T03:30:48Z

I would suggest we look to finalise the discussion in #1104 especially around scenarios & use cases prior to merging this request.

github-actions · 2024-06-29T03:20:15Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

heyams · 2024-07-01T22:51:38Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

please keep open.

heyams · 2024-07-02T18:40:31Z

@AlexanderWert can you please join next Monday's semcon SIG to discuss this? I will be there.

model/registry/user.yaml

Co-authored-by: Alexandra Konrad <[email protected]>

trisch-me · 2024-07-18T09:15:37Z

please update also markdown

trisch-me · 2024-07-29T14:21:42Z

@heyams let's wait for outcome of the discussion in the related issue
#1104 before continuing on this PR

model/registry/user.yaml

…s/semantic-conventions into heya/add-anonymous-user-id

.chloggen/add_authentication_user_subnamespace.yaml

heyams · 2024-08-06T20:48:04Z

i can't validate chlolg yaml locally.. stuck with this

.chloggen/add_authentication_user_subnamespace.yaml

model/general.yaml

model/registry/user.yaml

.chloggen/add_authentication_user_subnamespace.yaml

docs/attributes-registry/user.md

lmolkova · 2024-08-08T16:42:23Z

@AlexanderWert could you please take another look? I believe your feedback was addressed.

heyams · 2024-08-08T19:05:22Z

i checked that link is still live. is this false negative?

lmolkova · 2024-08-08T19:49:20Z

model/general.yaml

-      - ref: enduser.role
+      - ref: user.id
+        requirement_level:
+          conditionally_required: If instrumentation supports tracking unauthenticated users and if `user.authentication.id` is set, recommended otherwise.


sorry, I think I got it wrong in my previous suggestion.

We don't want user.id to be used for authenticated users, do we? so I guess it should be

Suggested change

conditionally_required: If instrumentation supports tracking unauthenticated users and if `user.authentication.id` is set, recommended otherwise.

conditionally_required: If and only if instrumentation supports tracking unauthenticated users.

wdyt?

I believe the goal is to prevent someone from duplicating things (or using user.id for authenticated one) like

❌ Bad:

user.authentication.id = lmolkova user.id = lmolkova

❌ Bad:

user.id = lmolkova

we want people to do

✅ Good:

user.id = QdH5CAWJgqVT4rOr0qtumf

✅ Good:

user.id = QdH5CAWJgqVT4rOr0qtumf user.authentication.id = lmolkova

✅ Good:

user.authentication.id = lmolkova

Maybe another way to same this is that this value SHOULD NOT be identifying in anyway to the real user id. So this value MUST not contain PII which can directly identify the authenticated user.

PII is once concern, but my main point is is avoid duplication (you should not generate new guid for the sake of populating user.id) and consistency - everyone should be using user.*.id in the same way

for azure monitor javascript SDK, browser always has a cookie named ai_user=QdH5CAWJgqVT4rOr0qtumf.
this will be tracked via user.id regardless of whether user is authenticated or not. user can choose to use user.authentication.id for the real user's id/name.

can we make it not required and remove the condition completely?

I'm missing something.

The condition here is if tracking unauthenticated users is supported, so azmon distro should set user.id based on it.

this condition says nothing about authentication.

all i'm saying that user.id is not a required field.

would this work?

Suggested change

conditionally_required: If instrumentation supports tracking unauthenticated users and if `user.authentication.id` is set, recommended otherwise.

recommended: If and only if instrumentation supports tracking unauthenticated users.

i'm saying that user.id is not a required field.

It might be not required for this specific use case. But for anything related to the users in OS user.id is well known and important field

lmolkova · 2024-08-08T19:57:44Z

model/registry/user.yaml

+          The `user.id`, when populated, is expected to be generated before user is authenticated and SHOULD NOT change after the user logs in.
+          In browser scenarios `user.id` is usually stored in cookies.
+
+          It's NOT RECOMMENDED to populate this attribute when unauthenticated users are not tracked or identified by the system.
+
+          It can be a random guid or a hash of the user's IP address. This is different from `user.hash` which is a hash of a known `user.id` or `user.name`.


sorry, one more minor suggestion to rearrange things slightly (feel free to rephrase or not take it at all)

Suggested change

The `user.id`, when populated, is expected to be generated before user is authenticated and SHOULD NOT change after the user logs in.

In browser scenarios `user.id` is usually stored in cookies.

It's NOT RECOMMENDED to populate this attribute when unauthenticated users are not tracked or identified by the system.

It can be a random guid or a hash of the user's IP address. This is different from `user.hash` which is a hash of a known `user.id` or `user.name`.

The `user.id`, when populated, is expected to be generated before user is authenticated and SHOULD NOT change after the user logs in.

It's NOT RECOMMENDED to populate this attribute when unauthenticated users are not tracked or identified by the system.

In browser scenarios `user.id` is usually stored in cookies. It can also be a random guid or a hash of the user's IP address. This is different from `user.hash` which is a hash of a known `user.authentication.id` or `user.name`.

docs/attributes-registry/user.md

MSNev · 2024-08-08T20:13:31Z

docs/attributes-registry/user.md

+**[2]:** Useful if `user.authentication.id` or `user.name` contain confidential information and cannot be used.
+
+**[3]:** The `user.id`, when populated, is expected to be generated before user is authenticated and SHOULD NOT change after the user logs in. In browser scenarios `user.id` is usually stored in cookies.
+It's NOT RECOMMENDED to populate this attribute when unauthenticated users are not tracked or identified by the system.


I would remove this line, OpenTelemetry should not be making these statement.

As it's up to the application to determine whether or not they want to track the user.id when the user in authenticated. For client applications this is almost ALWAYS required as they will use this to identify the "user" in some random way

why?

we want authenticated user id to always be in the same attribute user.authentication.id. If you don't have means to track unauthenticated users, you should never populate user.id.

Otherwise how you see people using those attributes?

only authenticated user id can be tracked through user.authentication.id and everything else is tracked through 'user.id', it can be anonymous, random, pseudo, unauthenticated, unauthorized and etc. can we not make any recommendation how OpenTelemetry customers use this attribute? as long as we have a crystal-clear description, it's up to the users to decide. does it help?

We're recommending to use user.id only if app has some means to identify them (without authentication),
Assuming app has no means to identify users before authentication, user.id should not be used.

App can identify them by generating guid on every call, that's also a way of identification/tracking.

So from what I can tell this NOT RECOMMENDED just reinforces the description and does not contradict anything you're saying.

I want to keep this sentence since I expect people to put things they don't need into user.id and want to do as much as possible to avoid it. What's the problem in keeping it?

I don't have a strong opinion on this; your argument seems valid. let's try to resolve it in Monday's SIG with @MSNev and everyone else.

I’m finding these names a bit confusing, and agree with this:

I expect people to put things they don't need into user.id and want to do as much as possible to avoid it

It seems that unauthenticated/tracking/pseudo user ids are fairly specific to client (browser/mobile) instrumentation, so maybe it’s better to have a more specific name for that concept (and not use user.id for it)?

MSNev · 2024-08-08T20:13:47Z

docs/general/attributes.md

+| [`user.roles`](/docs/attributes-registry/user.md) | string[] | Array of user roles at the time of the event. | `["admin", "reporting_user"]` | `Recommended` | ![Experimental](https://img.shields.io/badge/-experimental-blue) |
+
+**[1]:** The `user.id`, when populated, is expected to be generated before user is authenticated and SHOULD NOT change after the user logs in. In browser scenarios `user.id` is usually stored in cookies.
+It's NOT RECOMMENDED to populate this attribute when unauthenticated users are not tracked or identified by the system.


Again remove

MSNev · 2024-08-08T20:16:47Z

model/registry/user.yaml

+          The `user.id`, when populated, is expected to be generated before user is authenticated and SHOULD NOT change after the user logs in.
+          In browser scenarios `user.id` is usually stored in cookies.
+
+          It's NOT RECOMMENDED to populate this attribute when unauthenticated users are not tracked or identified by the system.


MSNev

Please see comments about being not recommended.

trisch-me · 2024-09-16T15:07:42Z

@heyams hey, do you have any updates for this issue? Can you ping me in slack if you have worked on it while I was away?

heyams · 2024-09-16T19:09:14Z

@heyams hey, do you have any updates for this issue? Can you ping me in slack if you have worked on it while I was away?

will do

lmolkova · 2024-10-14T17:43:00Z

Do we want to close this one in favor of #1456?

heyams added 2 commits June 11, 2024 16:27

Add user.anonymous_id

c809107

Format

1eb9c37

heyams requested review from a team June 11, 2024 23:31

github-actions bot assigned joaopgrassi Jun 11, 2024

Update changelog

1fd2648

MSNev reviewed Jun 11, 2024

View reviewed changes

model/registry/user.yaml Outdated Show resolved Hide resolved

Autogenerate changelog and markdown

db88ea7

trisch-me reviewed Jun 12, 2024

View reviewed changes

model/registry/user.yaml Outdated Show resolved Hide resolved

AlexanderWert requested changes Jun 12, 2024

View reviewed changes

model/registry/user.yaml Outdated Show resolved Hide resolved

Add note to user.anonymous_id

58a1096

scheler approved these changes Jun 12, 2024

View reviewed changes

reyang reviewed Jun 12, 2024

View reviewed changes

model/registry/user.yaml Outdated Show resolved Hide resolved

Update anonymous_id brief

3dfb971

lmolkova mentioned this pull request Jun 20, 2024

When someone should use user.name/user.id ? #1172

Open

github-actions bot added the Stale label Jun 29, 2024

lmolkova removed the Stale label Jul 2, 2024

trisch-me added the never stale PRs marked with this label will be never staled and automatically closed label Jul 16, 2024

heyams commented Jul 16, 2024

View reviewed changes

model/registry/user.yaml Outdated Show resolved Hide resolved

trisch-me reviewed Jul 17, 2024

View reviewed changes

model/registry/user.yaml Outdated Show resolved Hide resolved

Comment

6731337

Co-authored-by: Alexandra Konrad <[email protected]>

Update markdown

f2c4782

heyams requested a review from a team July 22, 2024 15:30

Merge branch 'main' into heya/add-anonymous-user-id

6419885

heyams commented Aug 6, 2024

View reviewed changes

model/registry/user.yaml Outdated Show resolved Hide resolved

heyams and others added 4 commits August 6, 2024 11:24

Merge branch 'main' into heya/add-anonymous-user-id

04f56e1

Update generate identity markdown

1bc5f88

Merge branch 'heya/add-anonymous-user-id' of https://github.com/heyam…

8c81fd1

…s/semantic-conventions into heya/add-anonymous-user-id

Update changelog

443f497

heyams commented Aug 6, 2024

View reviewed changes

.chloggen/add_authentication_user_subnamespace.yaml Outdated Show resolved Hide resolved

Fix yamllint

63af008

Fix make command error

615ff2c

heyams changed the title ~~Add anonymous_id to user namespace~~ Add authentication sub-namespace to user Aug 6, 2024

Update id

81d6360

MSNev approved these changes Aug 7, 2024

View reviewed changes

lmolkova reviewed Aug 7, 2024

View reviewed changes

heyams added 2 commits August 8, 2024 11:48

Address comments

bf8e35e

Fix yamllint

6f7dc08

lmolkova reviewed Aug 8, 2024

View reviewed changes

docs/attributes-registry/user.md Outdated Show resolved Hide resolved

MSNev reviewed Aug 8, 2024

View reviewed changes

MSNev requested changes Aug 8, 2024

View reviewed changes

Update user.hash referrence to user.authentication.id everywhere

5a6d337

mjwolf added the area:security label Sep 19, 2024

heyams closed this Oct 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authentication sub-namespace to user #1146

Add authentication sub-namespace to user #1146

heyams commented Jun 11, 2024

thompson-tomo commented Jun 13, 2024

github-actions bot commented Jun 29, 2024

heyams commented Jul 1, 2024

heyams commented Jul 2, 2024

trisch-me commented Jul 18, 2024

trisch-me commented Jul 29, 2024

heyams commented Aug 6, 2024

lmolkova commented Aug 8, 2024

heyams commented Aug 8, 2024

lmolkova Aug 8, 2024 •

edited

Loading

lmolkova Aug 8, 2024 •

edited

Loading

MSNev Aug 8, 2024

lmolkova Aug 8, 2024

heyams Aug 8, 2024 •

edited

Loading

lmolkova Aug 8, 2024 •

edited

Loading

heyams Aug 8, 2024

lmolkova Aug 8, 2024

trisch-me Aug 12, 2024

lmolkova Aug 8, 2024

MSNev Aug 8, 2024

lmolkova Aug 8, 2024 •

edited

Loading

heyams Aug 8, 2024

lmolkova Aug 9, 2024 •

edited

Loading

heyams Aug 9, 2024

trask Aug 11, 2024

MSNev Aug 8, 2024

MSNev Aug 8, 2024

MSNev left a comment

trisch-me commented Sep 16, 2024

heyams commented Sep 16, 2024

lmolkova commented Oct 14, 2024

	conditionally_required: If instrumentation supports tracking unauthenticated users and if `user.authentication.id` is set, recommended otherwise.
	conditionally_required: If and only if instrumentation supports tracking unauthenticated users.

	conditionally_required: If instrumentation supports tracking unauthenticated users and if `user.authentication.id` is set, recommended otherwise.
	recommended: If and only if instrumentation supports tracking unauthenticated users.

Add authentication sub-namespace to user #1146

Add authentication sub-namespace to user #1146

Conversation

heyams commented Jun 11, 2024

thompson-tomo commented Jun 13, 2024

github-actions bot commented Jun 29, 2024

heyams commented Jul 1, 2024

heyams commented Jul 2, 2024

trisch-me commented Jul 18, 2024

trisch-me commented Jul 29, 2024

heyams commented Aug 6, 2024

lmolkova commented Aug 8, 2024

heyams commented Aug 8, 2024

lmolkova Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

lmolkova Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyams Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

lmolkova Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmolkova Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmolkova Aug 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSNev left a comment

Choose a reason for hiding this comment

trisch-me commented Sep 16, 2024

heyams commented Sep 16, 2024

lmolkova commented Oct 14, 2024

lmolkova Aug 8, 2024 •

edited

Loading

lmolkova Aug 8, 2024 •

edited

Loading

heyams Aug 8, 2024 •

edited

Loading

lmolkova Aug 8, 2024 •

edited

Loading

lmolkova Aug 8, 2024 •

edited

Loading

lmolkova Aug 9, 2024 •

edited

Loading