Feat: Dynamic sampling by marandaneto · Pull Request #1004 · getsentry/sentry-dart

marandaneto · 2022-09-13T07:31:40Z

📜 Description

https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/

💡 Motivation and Context

https://docs.sentry.io/product/data-management-settings/server-side-sampling/

Issues addressed:
Similar to #604 but with a different name https://develop.sentry.dev/sdk/performance/#tracepropagationtargets getsentry/team-mobile#39
getsentry/team-mobile#44
getsentry/team-mobile#30

💚 How did you test it?

Not yet

📝 Checklist

🔮 Next steps

codecov-commenter · 2022-09-13T09:10:32Z

Codecov Report

❌ Patch coverage is 93.97590% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.91%. Comparing base (330bd1a) to head (6c8fac4).
⚠️ Report is 1629 commits behind head on main.

Files with missing lines	Patch %	Lines
dart/lib/src/sentry_baggage.dart	90.90%	6 Missing ⚠️
dart/lib/src/protocol/sentry_transaction_info.dart	70.00%	3 Missing ⚠️
dart/lib/src/noop_sentry_span.dart	33.33%	2 Missing ⚠️
dart/lib/src/hub_adapter.dart	0.00%	1 Missing ⚠️
...b/src/protocol/sentry_transaction_name_source.dart	85.71%	1 Missing ⚠️
dart/lib/src/sentry_traces_sampler.dart	87.50%	1 Missing ⚠️
dart/lib/src/sentry_transaction_context.dart	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1004      +/-   ##
==========================================
+ Coverage   89.66%   89.91%   +0.24%     
==========================================
  Files         106      113       +7     
  Lines        3329     3539     +210     
==========================================
+ Hits         2985     3182     +197     
- Misses        344      357      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

marandaneto · 2022-09-20T07:03:03Z

  meta: ^1.3.0
  stack_trace: ^1.10.0
  uuid: ^3.0.0
+  intl: ^0.17.0 # TODO remove or vendor it (its a dart.dev lib)


See https://github.com/getsentry/sentry-dart/pull/1004/files#r974958847
We can either vendor this class or accept it, its a lib maintained by the Dart team, 19 months no new releases

Since these decisions can be more 'platform' specific, I'll defer this to you and community. We've had a few dependencies so far without issues.

Is this a dependency most users would add anyways? If so I'd say it's fine to add if we expect to need more stuff from it. If it's just the number formatting for APIs maybe we should replace it with custom code instead? Basically we just need to convert the double to a string that doesn't use exponent notation.

Since it's not part of the standard lib, I'd say so, it's an official lib btw.
Dart does code stripping so bundle size is also not a concern.
The class that does it is quite big and complex, and obviously handles more cases.
https://stackoverflow.com/questions/62989638/convert-long-double-to-string-without-scientific-notation-dart
I'd rather use the lib then so it works on every platform without no surprises, including JS.

marandaneto · 2022-09-20T07:04:12Z

@@ -1,5 +1,5 @@
 # Uncomment this line to define a global platform for your project
-platform :ios, '9.0'
+platform :ios, '11.0'


Because of the new Xcode, it does it automatically, but its just the sample.

brustolin · 2022-09-20T08:35:26Z

Everything looks good. Just need to check why CI is not happy.

adinauer

Left some comments and questions.

adinauer · 2022-09-20T07:58:18Z

  meta: ^1.3.0
  stack_trace: ^1.10.0
  uuid: ^3.0.0
+  intl: ^0.17.0 # TODO remove or vendor it (its a dart.dev lib)


Is this a dependency most users would add anyways? If so I'd say it's fine to add if we expect to need more stuff from it. If it's just the number formatting for APIs maybe we should replace it with custom code instead? Basically we just need to convert the double to a string that doesn't use exponent notation.

adinauer · 2022-09-20T09:16:42Z

+    for (final keyValueString in keyValueStrings) {
+      // TODO: Note, value MAY contain any number of the equal sign (=) characters.
+      // Parsers MUST NOT assume that the equal sign is only used to separate key and value.
+      final keyAndValue = keyValueString.split('=');


Java SDK now looks for the first = and splits there instead of splitting on every =. I think this was discussed internally to not have to be changed immediately tho.

yes, not for now, no SDK handles that so far, so it's a separate issue/TODO.

marandaneto · 2022-09-20T13:46:34Z

Ignore sentry-flutter / package-analysis (pull_request), this is intended, its a bug in the analyzer.

brustolin

LGTM

github-actions · 2022-09-21T13:00:30Z

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

Generated by 🚫 dangerJS against 6c8fac4

Feat: Dynamic sampling

7d9d368

marandaneto mentioned this pull request Sep 13, 2022

Support Transaction info source getsentry/team-mobile#30

Closed

6 tasks

fix

9976ed7

marandaneto added 4 commits September 13, 2022 14:15

add sampling decision

4b70093

replace sampled to sampling decision

57687de

adding tracing origins

e89bd42

fixes and add tests for tracing utils

e9cb38f

marandaneto commented Sep 14, 2022

View reviewed changes

Comment thread dart/lib/src/utils/tracing_utils.dart

AbhiPrasad reviewed Sep 14, 2022

View reviewed changes

Comment thread dart/lib/src/utils/tracing_utils.dart Outdated

marandaneto added 7 commits September 14, 2022 17:35

add max number of length and items

7ee3a33

fix tests

a7b584f

tests

8021819

trace context fixes

254d9a9

fix

93b4e04

add more tests

1aa3ad7

fix test

8e1fdea

marandaneto commented Sep 19, 2022

View reviewed changes

Comment thread flutter/example/lib/main.dart Outdated

generate mocks

edd6067

marandaneto commented Sep 20, 2022

View reviewed changes

Comment thread dart/lib/src/sentry_baggage.dart Outdated

marandaneto commented Sep 20, 2022

View reviewed changes

Comment thread dart/lib/src/sentry_tracer.dart Outdated

marandaneto commented Sep 20, 2022

View reviewed changes

Comment thread dart/lib/src/utils/tracing_utils.dart Outdated

marandaneto commented Sep 20, 2022

View reviewed changes

marandaneto requested a review from adinauer September 20, 2022 07:05

marandaneto added 2 commits September 20, 2022 10:54

fix

887b64b

add changelog

8215f34

adinauer reviewed Sep 20, 2022

View reviewed changes

fix overwriting sentrys header

b204f5c

marandaneto added 5 commits September 20, 2022 14:14

Merge branch 'main' into feat/dynamic-sampling

0399375

fix readme

ae3c436

fix

0d70a09

revert test

bb49d2c

add logging

31a6842

marandaneto marked this pull request as ready for review September 20, 2022 12:50

fix enums name

12954b0

marandaneto requested review from adinauer and brustolin September 20, 2022 13:32

brustolin approved these changes Sep 20, 2022

View reviewed changes

adinauer approved these changes Sep 20, 2022

View reviewed changes

marandaneto mentioned this pull request Sep 20, 2022

Support tracingOrigins for performance feature #604

Closed

Merge branch 'main' into feat/dynamic-sampling

5a89598

marandaneto and others added 3 commits September 22, 2022 12:57

Merge branch 'main' into feat/dynamic-sampling

3503330

fix changelog

da509aa

Merge branch 'main' into feat/dynamic-sampling

6c8fac4

marandaneto merged commit 9835e1c into main Sep 22, 2022

marandaneto deleted the feat/dynamic-sampling branch September 22, 2022 11:57

This was referenced Sep 22, 2022

Adds Dart and Flutter SDK to supported SDKs for DS getsentry/sentry-docs#5545

Merged

Add Dart and Flutter SDK to supported SDKs - DS getsentry/sentry#39147

Merged

Uh oh!

Uh oh!

Conversation

marandaneto commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

codecov-commenter commented Sep 13, 2022 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brustolin commented Sep 20, 2022

Uh oh!

adinauer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marandaneto commented Sep 20, 2022

Uh oh!

brustolin left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marandaneto commented Sep 13, 2022 •

edited

Loading

codecov-commenter commented Sep 13, 2022 •

edited by codecov Bot

Loading

github-actions Bot commented Sep 21, 2022 •

edited

Loading