-
Notifications
You must be signed in to change notification settings - Fork 282
Add Browser Resource Timing Event #1943
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
Conversation
| - ref: url.full | ||
| requirement_level: required | ||
| note: > | ||
| Generally the url doesn't include any query strings or any leading credentials, |
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.
@open-telemetry/specs-semconv-maintainers do you see any issue with using url.full for something that doesn't include the query string?
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.
Some platforms use querystring params to resize images. If you strip it down to just domain and path, you won't have a complete picture of which size assets loaded. I understand the high cardinality issue but wanted to call this case out.
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.
Should we include query string separately?
|
Would it make sense to use integer nanosecond timestamps to be consistent timestamp/duration measurement in the rest of OpenTelemetry Looking beyond the browser, these are potentially applicable to a native app scenario via vendor APIs like URLSessionTaskTransactionMetrics on iOS. |
hey @ladd 👋 I think this event is a bit different because we're bridging an external event as-is, and so we are prioritizing preserving the external event structure/names/values over consistency with normal semantic conventions
@open-telemetry/android-approvers wdyt? @MSNev @martinkuba how important is it for this event to exactly match the browser event? or would it be ok as long as you have the ability to capture the same set of data, but it was modeled as a generic event with some potentially browser specific attributes on top of it? |
|
For the changelog, it will reuse and update the one from this PR #1940 |
The case for keeping as isAs this is the Even in OpenTelemetry there is existing code (hacks) to capture these currently as "Span" events https://github.com/open-telemetry/opentelemetry-js/blob/b322a24b4a3296577fb908c0bc9b33efc57c501e/packages/opentelemetry-sdk-trace-web/src/utils.ts#L63-L127 which record the values as is (which for most browsers will be a zero based "duration" (which is also why the hack to send as a span event (long before logs existed)) The case for Making ConsistentNow if this was (or becomes) a non-browser specific event definition and a general OpenTelemetry Semantic convention for all consumers of different types of "resource" types, then I would 100% agree with aligning and recording as nanoseconds. As a differently named event. General Background DetailsAs a general comment, in the browser world nothing is generally recorded in nanoseconds and when it is supported (as a double - as fractional microseconds) it is generally deliberately crippled (by adding random time "adjustments" with both positive and negative values) to avoid being used to "fingerprint" (identify / classify) a specific user / environment. These "adjustments" where added because this was occurring in the real world (with techniques like Canvas fingerprinting) |
| If the page starts unloading before the page loads, | ||
| then the partial data can be captured by calling the [performance.getEntriesByType](https://developer.mozilla.org/en-US/docs/Web/API/Performance/getEntriesByType) method. | ||
| attributes: | ||
| - ref: url.full |
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 think this URL is for the resource described by this event, not the page URL, correct? I think we should note this in the description of this attribute, so that it is clear.
Also, if this is the resource URL, why is it listed as an attribute rather than a body field like the measurements?
|
All fields that are available from the browser API are described here This event includes only a subset. I recall we discussed including only the most relevant fields a long time ago in the Client SIG. Now, after another review, I think there are additional fields we might want to consider including, for example:
This PR has all fields marked as recommended. If we included all the fields and made them all recommended (not required), it could be left to the instrumentation to determine which to include. This could also be configurable, so that users can decide what fields they care about. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
Could we re-open this PR? |
|
This is PR is under open status, still making changes to this PR |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
@hectorhdzg will take over this PR and continue addressing comments after #1943 (comment) or he will open a new PR based on this PR. |
|
close this PR since #3069 is opened for the following changes. Thanks @hectorhdzg |
Fixes #
Changes
Please provide a brief description of the changes here.
Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.
Merge requirement checklist
[chore]