Skip to content

Conversation

devin-ai-integration[bot]
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Sep 5, 2025

Summary

Note: This PR direction changed significantly during development.

Initially added HTTP/URL resource attributes (url.scheme, url.domain, url.path, url.query, url.full, http.route, url.port) to the OpenTelemetry browser resource configuration. However, after receiving feedback pointing out conflicts with existing span attributes and investigating OpenTelemetry standards, I removed all URL resource attributes to ensure compliance.

Key findings:

  • OpenTelemetry browser resource conventions do not include URL attributes (only browser.brands, browser.language, etc.)
  • HTTP semantic conventions define URL attributes for spans/metrics only, not resources
  • The same attribute keys were being used for both resources (static, from window.location) and spans (dynamic, from HTTP requests), creating semantic conflicts
  • Existing enhanceSpanWithHttpRequestAttributes function correctly implements URL attributes for spans

Current state: Resource configuration now only includes standard browser resource attributes per OpenTelemetry conventions. URL information remains available through existing span attributes.

How did you test this change?

  • yarn format:all passed
  • ⚠️ Full test suite was hanging, but Next.js integration tests completed successfully
  • ✅ Code follows existing patterns and removes conflicting attributes
  • ✅ Verified existing span attribute implementation remains unchanged

Are there any deployment considerations?

No deployment considerations. This change only affects the OpenTelemetry resource configuration in the browser SDK and maintains backward compatibility.


Human Review Checklist:

  • Verify my interpretation of OpenTelemetry standards is correct
  • Consider if there are alternative approaches to add valuable browser resource attributes that are standards-compliant
  • Confirm existing span attributes provide sufficient observability for the original use case
  • Check if removing URL resource attributes affects any downstream telemetry consumers

Link to Devin run: https://app.devin.ai/sessions/a7d3c99d04b9438984d7997b4deb1950
Requested by: @Vadman97

- Add url.scheme, url.domain, url.path, url.query, url.full attributes
- Add url.port when available
- Based on OpenTelemetry semantic conventions
- Extracted from current browser location (window.location)

Co-Authored-By: Vadim Korolik <vkorolik@gmail.com>
@devin-ai-integration devin-ai-integration bot requested a review from a team as a code owner September 5, 2025 20:06
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Contributor Author

Closing due to inactivity for more than 7 days. Configure here.

@Vadman97 Vadman97 reopened this Sep 25, 2025
@ccschmitz-launchdarkly
Copy link
Contributor

@Vadman97 what are we hoping to get from this? Will things like path and query be useful as resource attributes since they are static and won't change after initialization? Also, will any of these conflict with the attributes we try to apply in our browser instrumentations? We already try to apply some of these attributes in our browser instrumentation too - would there be any conflicts with these?

- Remove conflicting URL attributes from resource configuration
- URL attributes belong in spans per OpenTelemetry HTTP semantic conventions
- Resolves conflict with existing span attributes in enhanceSpanWithHttpRequestAttributes
- Follows OpenTelemetry browser resource conventions which don't include URL attributes

Co-Authored-By: Vadim Korolik <vkorolik@gmail.com>
@Vadman97 Vadman97 closed this Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants