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

ref(spans): Introduce opinionated casing in span metrics #2193

Merged
merged 6 commits into from
Jun 7, 2023

Conversation

iker-barriocanal
Copy link
Contributor

@iker-barriocanal iker-barriocanal commented Jun 7, 2023

In the past, consistent and logical input in certain parameters was assumed. For example, an organization could send SELECT or select consistently, but combining them with other alternatives such as sELEct was not considered. The combination of these cases contributes to higher cardinality and degrades the product experience. In an attempt to improve both, this PR introduces opinionated casing for some fields. This decision is not locked-in and may change in the future as needs evolve.

The following cases are enforced:

  • Transaction method, uppercase. Example: GET.
  • Transaction operation, lowercase. Example: measure.
  • Span operation, lowercase. Example: ui.react.render.
  • Span action, uppercase for both HTTP and database spans. Examples: POST, SELECT.
  • Span domain, lowercase for both HTTP and database spans. Examples: mydomain, tablename.
  • Span system, lowercase. Example: mysql.

The following are NOT enforced:

  • Environment, to reuse the same as in transaction data.
  • Transaction name, since casing is important.
  • Span description, since casing is important.
  • Span group, the MD5 hash remains unchanged.

#skip-changelog

@iker-barriocanal iker-barriocanal requested a review from a team June 7, 2023 11:12
@iker-barriocanal iker-barriocanal self-assigned this Jun 7, 2023
"transaction": "GET /api/:version/users/",
"transaction.op": "myop",
"transaction": "gEt /api/:version/users/",
"transaction.op": "mYOp",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this test contains mixed metrics from transactions and spans. This PR only changes span metrics.

Copy link
Contributor

@olksdr olksdr left a comment

Choose a reason for hiding this comment

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

It looks good, but why did you opt into use to_ascii_ equivalents of the lower- and uppercase functions? I do not see the reason or benefits in doing so.
Could you, please, explain?

@iker-barriocanal
Copy link
Contributor Author

@olksdr you're right, there's no relevant reason to do so. Updated in 6ce20af. Thanks for pointing it out!

@iker-barriocanal iker-barriocanal requested a review from olksdr June 7, 2023 12:21
@iker-barriocanal iker-barriocanal merged commit 9dc38ee into master Jun 7, 2023
@iker-barriocanal iker-barriocanal deleted the iker/ref/spans-opinionated-casing branch June 7, 2023 15:20
jan-auer added a commit that referenced this pull request Jun 14, 2023
* master: (32 commits)
  feat(txname): Mark all URL transactions as sanitized (#2210)
  fix(profiles): Enforce profile quotas on metrics buckets (#2212)
  revert(spans): Move span metrics back to transactions namespace (#2206)
  instr(metrics): Add values to invalid metric errors (#2201)
  ref(spans): Move span metrics to spans namespace (#2205)
  chore(protocol): Add missing py/changelog entry for lock property (#2203)
  build(py): Move pytest.ini to tests directory (#2204)
  ref: Use serde collect_str to avoid allocations (#2149)
  chore(gocd): Auto approve deploy relay-pop on check success (#2200)
  test(document-pii): Fix flaky test (#2198)
  feat(spans): Extract transaction `user` into span's databag (#2197)
  feat(spans): Extract `release` tag from the transaction to the span (#2189)
  ref(metrics-extraction): Split span and transaction metric extraction (#2191)
  ref(crons): Support GET in the check-in endpoint (#2192)
  ref(spans): Introduce opinionated casing in span metrics (#2193)
  chore(py): Bump `requests` lib version (#2194)
  doc: document pii fields (#1972)
  feat: Added cron specific endpoint (#2153)
  feat(profiling): Extract app identifier from app context (#2172)
  feat(ci): Add CI-check for pop-deploys  (#2184)
  ...
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