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

feat: Add sample rate in the baggage header, remove Userid and Transaction #1936

Merged
merged 19 commits into from Jul 7, 2022

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Jul 4, 2022

📜 Description

Included the sample rate info in the baggage header.
Updated baggage header keys.
Checking PII before sharing user info in the baggage header.

💡 Motivation and Context

In preparation to dynamic sampling

💚 How did you test it?

Unit tests

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link

github-actions bot commented Jul 4, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 4be64fb

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM, from an ObjC level, have no clue if this aligns with the dynamic sampling spec. Can someone else have a look, @brustolin? We still miss tests.

@brustolin brustolin changed the title feat: Add sample rate in the baggage header feat: Add sample rate in the baggage header, remove Userid and Transaction Jul 6, 2022
@brustolin brustolin marked this pull request as ready for review July 6, 2022 11:27
@brustolin
Copy link
Contributor Author

Hey @lforst, @Lms24 can you guys take a look at this to see if it conforms to the specs we have right now?

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Disclaimer: I have no idea about Objective C so take this with a grain of salt.

From what I saw when going through the PR, I think the names in the envelope header and the baggage Http header are correct. (Just to be sure, they should only differ by the sentry- prefix.)

Otherwise, this seems to conform to the spec (in the sense that we do need the sample_rate and we're not including user_id and transaction for the time being).

Comment on lines +48 to +49
if (_sampleRate != nil)
[information setValue:_sampleRate forKey:@"sentry-sample_rate"];
Copy link
Member

Choose a reason for hiding this comment

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

Are we making sure here that the sentry-sample_rate is set as a simple decimal number (no exponential notation, NaN [if that's a thing in obj-c], etc)? (this applies to both, baggage and envelope header)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kind yes, because OBJ-C converter does not use exponential notation and dont have the NaN concept. So I would say we are good here.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome!

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM

@brustolin brustolin merged commit 34a5809 into master Jul 7, 2022
@brustolin brustolin deleted the feat/baggage-update branch July 7, 2022 15:30
kevinrenskers added a commit that referenced this pull request Jul 8, 2022
* master:
  build: Remove trailing-whitespace (#1953)
  ci: Compile iOS13-Swift sample (#1951)
  release: 7.20.0
  meta: Fix changelog (#1950)
  feat: Add sample rate in the baggage header, remove Userid and Transaction (#1936)
  build: Upate Brewfile.lock and Gemfile.lock (#1947)
  meta: Add Pre Commit Hook (#1946)
  ref: Remove unused SentryCrashDeadlock (#1941)
  feat: Add screenshot at crash (#1920)
  Add code docs for SentryScope (#1942)
kevinrenskers added a commit that referenced this pull request Jul 8, 2022
* master:
  build: Remove trailing-whitespace (#1953)
  ci: Compile iOS13-Swift sample (#1951)
  release: 7.20.0
  meta: Fix changelog (#1950)
  feat: Add sample rate in the baggage header, remove Userid and Transaction (#1936)
  build: Upate Brewfile.lock and Gemfile.lock (#1947)
@marandaneto
Copy link
Contributor

@brustolin this is missing.
We have to add the transaction back.

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.

None yet

5 participants