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

Refactor traces attributes #1140

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Refactor traces attributes #1140

merged 1 commit into from
Dec 15, 2023

Conversation

ka3de
Copy link
Collaborator

@ka3de ka3de commented Dec 15, 2023

What?

Rename traces attributes.

Why?

To better comply with conventions on attribute naming recommended by Open Telemetry. See docs.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes
  • I have commented on my code, particularly in hard-to-understand areas

Related PR(s)/Issue(s)

#1100

@ka3de
Copy link
Collaborator Author

ka3de commented Dec 15, 2023

I'm considering whether we should also change the "navigation span" here so the actual span name is navigation and the URL stays just as the attribute.
I have some doubts on this, as our initial traces/timeline representation is kind of "atypical" and we used this span to organize the rest, but I think this would be the most correct, and we can still represent it in whatever way we want in our UI.
Let me know what do you think.

@ka3de ka3de marked this pull request as ready for review December 15, 2023 12:05
@ka3de ka3de self-assigned this Dec 15, 2023
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Rename traces attributes to better comply with conventions on attribute
naming recommended by Open Telemetry.
@ka3de ka3de merged commit 7cc9ea7 into main Dec 15, 2023
17 checks passed
@ka3de ka3de deleted the refactor/traces-attrs branch December 15, 2023 13:48
ka3de added a commit that referenced this pull request Dec 15, 2023
Based on discussions in #1140 , a decision was made to, instead of using
the URL as the span name for navigation spans, use the word 'navigation'
to identify the action, following the same pattern as the rest of the
spans. Due to this, and because a navigation span will always have that
fixed span name, it is no longer necessary to accept the span name as
input for the method.
@ka3de ka3de mentioned this pull request Dec 15, 2023
3 tasks
@ka3de ka3de added the refactor label Dec 15, 2023
@ka3de
Copy link
Collaborator Author

ka3de commented Dec 15, 2023

I'm considering whether we should also change the "navigation span" here so the actual span name is navigation and the URL stays just as the attribute.

This will be done in a separate PR. See #1141

ka3de added a commit that referenced this pull request Dec 18, 2023
Based on discussions in #1140 , a decision was made to, instead of using
the URL as the span name for navigation spans, use the word 'navigation'
to identify the action, following the same pattern as the rest of the
spans. Due to this, and because a navigation span will always have that
fixed span name, it is no longer necessary to accept the span name as
input for the method.
ka3de added a commit that referenced this pull request Dec 19, 2023
Based on discussions in #1140 , a decision was made to, instead of using
the URL as the span name for navigation spans, use the word 'navigation'
to identify the action, following the same pattern as the rest of the
spans. Due to this, and because a navigation span will always have that
fixed span name, it is no longer necessary to accept the span name as
input for the method.
ka3de added a commit that referenced this pull request Dec 19, 2023
Based on discussions in #1140 , a decision was made to, instead of using
the URL as the span name for navigation spans, use the word 'navigation'
to identify the action, following the same pattern as the rest of the
spans. Due to this, and because a navigation span will always have that
fixed span name, it is no longer necessary to accept the span name as
input for the method.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants