(pkg/ottl) Add extracted OS info from UserAgent #35886
Conversation
github-actions
bot
requested review from
bogdandrutu,
evan-bradley,
kentquirk and
TylerHelmuth
ioandr
force-pushed
the
feature-i-useragent-osinfo
branch
from
fc52236 to
dcdbd5b
Compare
pkg/ottl/ottlfuncs/func_useragent.go
Outdated
| semconv.AttributeOSName: parsedOS.Family, | ||
| semconv.AttributeOSVersion: parsedOS.ToVersionString(), |
There was a problem hiding this comment.
I think those attributes should be in the UserAgent namespace instead, but unluckily they are not available in semconv yet (open-telemetry/semantic-conventions#1434).
Meanwhile, could we create them manually?
| semconv.AttributeOSName: parsedOS.Family, | |
| semconv.AttributeOSVersion: parsedOS.ToVersionString(), | |
| "user_agent." + semconv.AttributeOSName: parsedOS.Family, | |
| "user_agent." +semconv.AttributeOSVersion: parsedOS.ToVersionString(), |
There was a problem hiding this comment.
Sure, this makes sense. Pushed a FIXUP for this with a comment to remove the hardcoded prefix once user agent OS attributes are added to semconv.
Feel free to resolve the conversation if the FIXUP covers this.
There was a problem hiding this comment.
Thanks @ioandr , FIXUP looks good to me. It seems I cannot "resolve the conversation" (no UI button), maybe because of the forced push? 🤔
There was a problem hiding this comment.
Unless the semconv sig is waiting for proof-of-concept we should wait until the semconv sig as accepted them as experimental.
ioandr
force-pushed
the
feature-i-useragent-osinfo
branch
from
73d7a02 to
a06577d
Compare
ioandr
force-pushed
the
feature-i-useragent-osinfo
branch
from
a06577d to
1dc8187
Compare
pkg/ottl/ottlfuncs/func_useragent.go
Outdated
| return map[string]any{ | ||
| semconv.AttributeUserAgentName: parsedUserAgent.Family, | ||
| semconv.AttributeUserAgentOriginal: userAgentString, | ||
| semconv.AttributeUserAgentVersion: parsedUserAgent.ToVersionString(), | ||
| // Remove hardcoded prefix when UserAgent OS attributes are added to semvconv | ||
| "user_agent" + semconv.AttributeOSName: parsedOS.Family, |
There was a problem hiding this comment.
Shouldn't these attribute's names be separated by a dot? It currently generates names like user_agentos.name and user_agentos.version.
There was a problem hiding this comment.
I think we could also make them local constants so we wouldn't need to concatenate them in every execution/tests.
There was a problem hiding this comment.
Nice catch! +1 to @edmocosta suggestions
|
I think we also need to add a changelog entry and to update the |
Add changelog entry and update docs as needed. Signed-off-by: Ioannis Androulidakis <androulidakis.ioannis@gmail.com>
ioandr
force-pushed
the
feature-i-useragent-osinfo
branch
from
1dc8187 to
600cd7e
Compare
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description
Add OS attributes about name and version from the (parsed) User Agent string.
Link to tracking issue
Fixes #35458
Testing
Run
TestUserAgentParser(), add new fields to expected map, add new testcases taken from https://github.com/ua-parser/uap-core/blob/master/tests/test_os.yaml.Documentation
N/A