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

Scope attribute for browser page url in all spans and events emitted from browser #3222

Closed
1 of 2 tasks
scheler opened this issue Sep 2, 2022 · 9 comments
Closed
1 of 2 tasks
Labels

Comments

@scheler
Copy link
Contributor

scheler commented Sep 2, 2022

  • This only affects the JavaScript OpenTelemetry library
  • This may affect other libraries, but I would like to get opinions here first
  1. For the xml-http-request and fetch spans, we need an attribute to indicate the URL of the webpage initiating those calls. Since there can be many of such calls from a single webpage it makes sense to include this URL only once. In the absence of Ephemeral Resource Attributes, I suggest we add this as an attribute at the scope level. This means, the initiating page's URL is sent once for each batch of xhr and fetch spans exported.

  2. What should we call this attribute? We could use http.referrer as the browsers send this URL in Referer header for these calls, however, it's conditionally present based on the referrer policy. We could still use http.referrer as the semantic convention for this attribute but there's a possibility that this could be considered as the http request header even when it was not sent in the xhr/fetch call. If it's not http.referrer, what other semantic convention could we use for this attribute?

@t2t2
Copy link
Contributor

t2t2 commented Sep 6, 2022

Clarify of what you mean by scope, I assume InstrumentationScope since it's the only currently existing scope with attributes? (ignoring context-scoped attributes proposal as it's a proposal as frozen as ephemeral resource attributes)


I think you should expand the scope of the issue from just xhr & fetch instrumentations, having current page URL is also relevant for user interactions (which page was the clicked on thing on), instrumenting data query libraries (graphql/apollo, prisma, feathers, socket.io-client (websocket), any in-house api client), instrumenting components on the page (ChartWidget on X page), but also errors & logs could use what url the error happened on

so now duplication for each instrumentation, as they all have individual InstrumentationScope

Then there's more implementation question of how to make sure it's included - would it have to be implemented on base instrumentation so it's available for all instrumentations (what if they don't want it, what if none want it - dead code), or is it a concern of every instrumentation to include logic for it (duplicated code, a contrib/third party/in-house instrumentation just forgets when skimming through specs or documentation, any concerns about each instrumentation having it's own monitoring causing an issue like #3029)

As current browser page URL can be changed via history API or location.hash I did a few searches in both oteps and spec but couldn't find if InstrumentationScope also has similar "mutability" limitations as Resource?

If that is once again something that shouldn't be mutated (and js implementation-wise, direct mutations of attributes object could cause issues where data with old url + url changes on scope before exporting means that all old data would get new url, thing to consider for actual implementation), then I believe every time a span is wanted TracerProvider should be asked for a Tracer with currently valid attributes? That's some awful DX


So I think simplicity wise way better to actually push ephemeral resource attributes to reality as it'd be a common attribute for browser data. So imagine:

import {useSessionId, useDocumentLocation, ...} from '@otel/a-good-web-experience';

const resProvider = new ResourceProvider(); // But probably an implementation that provides an easy update API

useSessionId(resProvider); // sets current session id, new resource when new session
useDocumentLocation(resProvider); // sets current url, new resource when current URL has changed
useUserAgent(resProvider); // well, doesn't have to update but hey, possibility if you don't want to do this ingest side
useCurrentUser(resProvider); // currently logged in user, could change with logout

as they're all standalone functions they could be way more easily treeshaken (= removed from bundle) when the user doesn't want it. compared to config option or a class method which can't be


  1. What should we call this attribute?

I do feel like http.referrer should be reserved for HTTP server usage, especially when scope is extended to non-http instrumentations (currently user interaction instr uses http.url, and I'm just gonna ignore that).

Maybe better as general rum/- sig topic but this might be valid for each language/environment to use what's more standard for them, eg. browsers have window.location and document.location to provide current URL, so one of those would be an obvious oh that's what it means, and mobile apps I believe have concept of screens? so they could have an attribute with "screen" in it

@scheler
Copy link
Contributor Author

scheler commented Sep 6, 2022

Discussed this with @t2t2 and @MSNev today - this feature applies to all browser instrumentations except document-load, and not just xhr and fetch.

Given that document-load is the only instrumentation that doesn't need it, it will be good to put this logic in base class specific to browsers - in InstrumentationBase class in platform/browser/instrumentation.js.

For document-load too, this scope attribute makes sense for iFrames - as the url of the parent page that embeds the iframe.

For the document-load of the top-most window, this scope attribute could have an empty value or be nonexistant.

For 2, we are thinking of calling this attribute browser.page.url.

@scheler
Copy link
Contributor Author

scheler commented Sep 6, 2022

@t2t2 on the topic of push ephemeral resource attributes to reality, I am wondering if we could make this work in increments. That is, the ephemeral resource attributes could be a more general version of scope attributes, which again is more general compared to the attributes at signal level.

@t2t2
Copy link
Contributor

t2t2 commented Sep 7, 2022

It'd make sense when considering attribute priority, tho am a bit afraid that once one solution is out there, someone will hardcode their processing to specific scope level and then be wondering why suddenly stuff broken after updating (scope -> resource might be considerable as breaking change?)

@dyladan
Copy link
Member

dyladan commented Sep 7, 2022

It'd make sense when considering attribute priority, tho am a bit afraid that once one solution is out there, someone will hardcode their processing to specific scope level and then be wondering why suddenly stuff broken after updating (scope -> resource might be considerable as breaking change?)

Moving an attribute from one attribute type to another is almost certainly going to be considered a breaking change unless the spec specifically calls out a fallback order to look for "missing" attributes in new places. I don't know if the spec maintainers would be happy with such a provision though.

@scheler
Copy link
Contributor Author

scheler commented Sep 7, 2022

The spec on mapping Instrumentation Scope Attributes to non-OTLP destinations specifies that the scope attributes SHOULD be recorded at the Span/LogRecord level - and there's a section in the scope attributes OTEP defining the attribute value precedence when an attribute is present in both the scope and the signal the attribute the signal takes precedence. Based on these two I inferred that it is NOT a breaking change if we move an attribute from a Span/LogRecord to its Scope, iff the attribute was present in all the Spans/LogRecords in the Scope.

I also created an issue open-telemetry/opentelemetry-specification#2774 to get the attribute value precedence added to the spec.

@scheler scheler changed the title Scope attribute for browser page url initiating xhr/fetch calls Scope attribute for browser page url in all spans and events emitted from browser Sep 8, 2022
@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Nov 14, 2022
@github-actions
Copy link

github-actions bot commented Dec 5, 2022

This issue was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 5, 2022
Repository owner moved this from Backlog to Done in Spec: Client SDK and Instrumentation Dec 5, 2022
@olee
Copy link

olee commented Nov 5, 2024

Is ther any standard defined for current browser url yet? I saw mention of browser.page.url but I'm not sure if that would be the correct attribute to go for.
Also, I'd be interested in not only the url, but actually the low cardinality route (like http.route) for clients, so maybe that could also be added as browser.page.route).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants