-
Notifications
You must be signed in to change notification settings - Fork 174
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
Add authentication sub-namespace to user #1146
Changes from 21 commits
c809107
1eb9c37
1fd2648
db88ea7
58a1096
3dfb971
6731337
f2c4782
6419885
c80b910
a47e5cd
53a121c
04f56e1
1bc5f88
8c81fd1
443f497
63af008
615ff2c
81d6360
bf8e35e
6f7dc08
5a6d337
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
# Use this changelog template to create an entry for release notes. | ||
# | ||
# If your change doesn't affect end users you should instead start | ||
# your pull request title with [chore] or use the "Skip Changelog" label. | ||
|
||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: enhancement | ||
|
||
# The name of the area of concern in the attributes-registry, (e.g. http, cloud, db) | ||
component: user | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: introduce subnamespace `user.authentication` with a new attribute `user.authentication.id` | ||
|
||
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. | ||
# The values here must be integers. | ||
issues: [1104] | ||
|
||
# (Optional) One or more lines of additional information to render under the primary note. | ||
# These lines will be padded with 2 spaces and then inserted directly into the document. | ||
# Use pipe (|) for multiline entries. | ||
subtext: Update `identity` attributes under general attribute doc. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -409,9 +409,19 @@ These attributes may be used for any operation with an authenticated and/or auth | |
|
||
| Attribute | Type | Description | Examples | [Requirement Level](https://opentelemetry.io/docs/specs/semconv/general/attribute-requirement-level/) | Stability | | ||
|---|---|---|---|---|---| | ||
| [`enduser.id`](/docs/attributes-registry/enduser.md) | string | Deprecated, use `user.id` instead. | `username` | `Recommended` | ![Deprecated](https://img.shields.io/badge/-deprecated-red)<br>Replaced by `user.id` attribute. | | ||
| [`enduser.role`](/docs/attributes-registry/enduser.md) | string | Deprecated, use `user.roles` instead. | `admin` | `Recommended` | ![Deprecated](https://img.shields.io/badge/-deprecated-red)<br>Replaced by `user.roles` attribute. | | ||
| [`enduser.scope`](/docs/attributes-registry/enduser.md) | string | Deprecated, no replacement at this time. | `read:message, write:files` | `Recommended` | ![Deprecated](https://img.shields.io/badge/-deprecated-red)<br>Removed. | | ||
| [`user.id`](/docs/attributes-registry/user.md) | string | Identifies a user interacting with a system regardless of user authentication status. This identifier may be unique only through best-effort means. [1] | `QdH5CAWJgqVT4rOr0qtumf` | `Conditionally Required` [2] | ![Experimental](https://img.shields.io/badge/-experimental-blue) | | ||
| [`user.authentication.id`](/docs/attributes-registry/user.md) | string | Unique identifier of an authenticated user in the system. [3] | `S-1-5-21-202424912787-2692429404-2351956786-1000` | `Recommended` | ![Experimental](https://img.shields.io/badge/-experimental-blue) | | ||
| [`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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again remove |
||
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`. | ||
|
||
**[2]:** If instrumentation supports tracking unauthenticated users and if `user.authentication.id` is set, recommended otherwise. | ||
|
||
**[3]:** The `user.authentication.id` MAY be used to identify a user attempting to authenticate if it's known at this stage. | ||
|
||
|
||
|
||
|
||
<!-- markdownlint-restore --> | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -34,15 +34,16 @@ groups: | |||||||||
- ref: peer.service | ||||||||||
requirement_level: recommended | ||||||||||
- id: identity | ||||||||||
type: span | ||||||||||
type: attribute_group | ||||||||||
brief: > | ||||||||||
These attributes may be used for any operation with an authenticated and/or authorized enduser. | ||||||||||
These attributes may be used for any operation with an authenticated and/or authorized user. | ||||||||||
attributes: | ||||||||||
- ref: enduser.id | ||||||||||
requirement_level: recommended | ||||||||||
- 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. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, I think I got it wrong in my previous suggestion. We don't want
Suggested change
wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe the goal is to prevent someone from duplicating things (or using user.id for authenticated one) like ❌ Bad:
❌ Bad:
we want people to do ✅ Good:
✅ Good:
✅ Good:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for azure monitor javascript SDK, browser always has a cookie named can we make it not required and remove the condition completely? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm missing something. The condition here is if tracking unauthenticated users is supported, so azmon distro should set this condition says nothing about authentication. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all i'm saying that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would this work?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It might be not required for this specific use case. But for anything related to the users in OS |
||||||||||
- ref: user.authentication.id | ||||||||||
requirement_level: recommended | ||||||||||
- ref: enduser.scope | ||||||||||
- ref: user.roles | ||||||||||
heyams marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
requirement_level: recommended | ||||||||||
- id: thread | ||||||||||
type: span | ||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -22,14 +22,28 @@ groups: | |||||||||||||||||||||||
brief: > | ||||||||||||||||||||||||
Unique user hash to correlate information for a user in anonymized form. | ||||||||||||||||||||||||
note: > | ||||||||||||||||||||||||
Useful if `user.id` or `user.name` contain confidential information and cannot be used. | ||||||||||||||||||||||||
Useful if `user.authentication.id` or `user.name` contain confidential information and cannot be used. | ||||||||||||||||||||||||
examples: ['364fc68eaf4c8acec74a4e52d7d1feaa'] | ||||||||||||||||||||||||
- id: user.id | ||||||||||||||||||||||||
type: string | ||||||||||||||||||||||||
stability: experimental | ||||||||||||||||||||||||
brief: > | ||||||||||||||||||||||||
Unique identifier of the user. | ||||||||||||||||||||||||
Identifies a user interacting with a system regardless of user authentication status. This identifier may be unique only through best-effort means. | ||||||||||||||||||||||||
note: > | ||||||||||||||||||||||||
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. | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove |
||||||||||||||||||||||||
|
||||||||||||||||||||||||
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`. | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry, one more minor suggestion to rearrange things slightly (feel free to rephrase or not take it at all)
Suggested change
|
||||||||||||||||||||||||
examples: ['QdH5CAWJgqVT4rOr0qtumf'] | ||||||||||||||||||||||||
- id: user.authentication.id | ||||||||||||||||||||||||
type: string | ||||||||||||||||||||||||
brief: Unique identifier of an authenticated user in the system. | ||||||||||||||||||||||||
note: > | ||||||||||||||||||||||||
The `user.authentication.id` MAY be used to identify a user attempting to authenticate if it's known at this stage. | ||||||||||||||||||||||||
examples: ['S-1-5-21-202424912787-2692429404-2351956786-1000'] | ||||||||||||||||||||||||
stability: experimental | ||||||||||||||||||||||||
- id: user.name | ||||||||||||||||||||||||
type: string | ||||||||||||||||||||||||
stability: experimental | ||||||||||||||||||||||||
|
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 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
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.
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 populateuser.id
.Otherwise how you see people using those attributes?
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.
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?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.
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?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 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.
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’m finding these names a bit confusing, and agree with this:
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)?