Allow to set startTimestamp & endTimestamp manually to SentrySpan #676

fatihergin · 2021-12-22T10:36:02Z

📜 Description

Allows to set startTimestamp(while creating) and endTimestamp(while finishing) manually to SentrySpan

💡 Motivation and Context

It will allow to trace events which Sentry does not have control over starting point of that events.

e.g. When a 3rd party SDK returns only duration of an internal execution which is wanted to trace.

p.s. It's already implemented (startTimestamp) on sentry-java with the motivation of tracing app-start time where Sentry not initialized yet
It will give more control over transactions in terms of creating&finishing them.

It might be desired to decide whether to start a transaction or not conditionally which will be clear at a later time after starting the operation.

💚 How did you test it?

Unit tests

📝 Checklist

🔮 Next steps

vaind · 2022-01-03T08:29:59Z

Any thoughts/issues with this @marandaneto? We need this (as a sentry.io Business subscriber, if that makes any difference here :)

dart/lib/src/protocol/sentry_span.dart

codecov-commenter · 2022-01-03T13:43:55Z

Codecov Report

Merging #676 (651c5f6) into main (14203b9) will increase coverage by 5.50%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #676      +/-   ##
==========================================
+ Coverage   90.65%   96.15%   +5.50%     
==========================================
  Files          97        2      -95     
  Lines        3145       52    -3093     
==========================================
- Hits         2851       50    -2801     
+ Misses        294        2     -292

Impacted Files	Coverage Δ
dart/lib/src/hub.dart
dart/lib/src/hub_adapter.dart
dart/lib/src/noop_hub.dart
dart/lib/src/noop_sentry_span.dart
dart/lib/src/protocol/sentry_span.dart
dart/lib/src/sentry.dart
dart/lib/src/sentry_tracer.dart
dart/lib/src/diagnostic_logger.dart
dart/lib/src/protocol/sentry_culture.dart
dart/lib/src/sentry_client.dart
... and 84 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14203b9...651c5f6. Read the comment docs.

marandaneto · 2022-02-07T13:44:05Z

dart/lib/src/protocol/sentry_span.dart

+      _hub.options.logger(
+        SentryLevel.warning,
+        'End timestamp ($endTimestamp) cannot be before start timestamp ($_startTimestamp)',
+      );
+      _endTimestamp = getUtcDateTime();


This is a good idea, but getUtcDateTime() can still be technically before the given _startTimestamp.
This needs to be tried/tested and see if Sentry.io throws an ingestion error or just an UI warning, otherwise we have to fix on the SDK side.

AFAIU, Sentry JS simply discards spans if end timestamp is before start timestamp.
Should this approach be applicable here?

this.spanRecorder.spans = this.spanRecorder.spans.filter((span: Span) => { // If we are dealing with the transaction itself, we just return it if (span.spanId === this.spanId) { return true; } ... const keepSpan = span.startTimestamp < endTimestamp; if (!keepSpan) { logger.log( '[Tracing] discarding Span since it happened after Transaction was finished', JSON.stringify(span, undefined, 2), ); } return keepSpan; });

Yes, otherwise it'll be discarded in the event ingestion, so no need to send the data over.

I can't find a suitable place to filter invalid spans out.
Possible places:

in Hub.captureTransaction, before item.client.captureTransaction(transaction): can't modify transaction

in SentryTracer.finish, before _hub.captureTransaction(transaction): can't modify transaction

in constructor of SentryTransaction, while initializing spans: spans = _tracer.children.where(...):
#(3) might work, but smells. Any recommendation?

@marandaneto

@fatihergin I believe SentryTracer#finish is the way to go, within the for loop from L67-L73, just remove the child span from the _children list if the timestamp is out of bounds.

marandaneto · 2022-02-07T13:50:00Z

Any thoughts/issues with this @marandaneto? We need this (as a sentry.io Business subscriber, if that makes any difference here :)

Sorry late reply, I was on vacay back then and missed the comment.

marandaneto · 2022-02-07T13:51:31Z

@fatihergin since there were changes in the public API, please check the other packages within this repo, they might need to be fixed too, e.g. Hub.

marandaneto · 2022-02-07T13:51:56Z

@brustolin would you mind having a look at it?

dart/lib/src/protocol/sentry_span.dart

fatihergin · 2022-02-07T14:26:07Z

@fatihergin since there were changes in the public API, please check the other packages within this repo, they might need to be fixed too, e.g. Hub.

I think I did it for hub, hub_adapter, noop_hub and hub_test. It looks there is no other relation between SentrySpan's start & end timestamps and the other packages @marandaneto

marandaneto · 2022-02-07T14:36:46Z

@fatihergin since there were changes in the public API, please check the other packages within this repo, they might need to be fixed too, e.g. Hub.

I think I did it for hub, hub_adapter, noop_hub and hub_test. It looks there is no other relation between SentrySpan's start & end timestamps and the other packages @marandaneto

There are untouched packages by this PR, e.g. dio and logging
Those also have mocks of the Hub class and that's why it fails.
See the CI Checks for reference.

dart/lib/src/noop_sentry_span.dart

ueman · 2022-02-07T14:53:54Z

@fatihergin since there were changes in the public API, please check the other packages within this repo, they might need to be fixed too, e.g. Hub.

I think I did it for hub, hub_adapter, noop_hub and hub_test. It looks there is no other relation between SentrySpan's start & end timestamps and the other packages @marandaneto

There are untouched packages by this PR, e.g. dio and logging Those also have mocks of the Hub class and that's why it fails. See the CI Checks for reference.

That makes me wonder if those mocks should only override what's important to them and then just override noSuchMethod in order to ignore all other methods.

marandaneto · 2022-02-07T15:04:08Z

@fatihergin since there were changes in the public API, please check the other packages within this repo, they might need to be fixed too, e.g. Hub.

I think I did it for hub, hub_adapter, noop_hub and hub_test. It looks there is no other relation between SentrySpan's start & end timestamps and the other packages @marandaneto

There are untouched packages by this PR, e.g. dio and logging Those also have mocks of the Hub class and that's why it fails. See the CI Checks for reference.

That makes me wonder if those mocks should only override what's important to them and then just override noSuchMethod in order to ignore all other methods.

yep, that makes a lot of sense.

dart/lib/src/sentry_tracer.dart

marandaneto · 2022-03-03T08:10:00Z

Quick update getsentry/develop#508
Our docs now mention that passing a custom endTimestamp is fine, so we can move forward with this PR.
@fatihergin would you like to address the comments? thanks.

marandaneto · 2022-03-07T12:08:26Z

CHANGELOG.md


 * Fix: Disable log by default in debug mode (#753)
 * [Dio] Ref: Replace FailedRequestAdapter with FailedRequestInterceptor (#728)
+* Feat: Allow to set startTimestamp & endTimestamp manually to SentrySpan (#676)


This should go under the Unreleased entry

marandaneto · 2022-03-07T12:14:43Z

@fatihergin only missing #676 (comment) and fix the changelog entry, otherwise LGTM.

fatihergin requested review from brustolin, denrase and marandaneto as code owners December 22, 2021 10:36

fatihergin changed the title ~~Allow to set startTimestamp explicitly for SentrySpan~~ Allow to set startTimestamp & endTimestamp manually to SentrySpan Dec 22, 2021

fatihergin force-pushed the feat/allow-to-set-sentryspan-starttimestamp-explicitly branch from 693476e to 651c5f6 Compare December 22, 2021 21:49

bruno-garcia reviewed Jan 3, 2022

View reviewed changes

dart/lib/src/protocol/sentry_span.dart Outdated Show resolved Hide resolved

brustolin mentioned this pull request Jan 27, 2022

Feat: Support maxSpan for performance API and expose SentryOptions through Hub #716

Merged

5 tasks

fatihergin force-pushed the feat/allow-to-set-sentryspan-starttimestamp-explicitly branch 4 times, most recently from 00b1b87 to d0b0db3 Compare February 7, 2022 13:07

marandaneto reviewed Feb 7, 2022

View reviewed changes

dart/lib/src/protocol/sentry_span.dart Show resolved Hide resolved

marandaneto reviewed Feb 7, 2022

View reviewed changes

dart/lib/src/protocol/sentry_span.dart Outdated Show resolved Hide resolved

marandaneto reviewed Feb 7, 2022

View reviewed changes

dart/lib/src/protocol/sentry_span.dart Show resolved Hide resolved

marandaneto reviewed Feb 7, 2022

View reviewed changes

dart/lib/src/noop_sentry_span.dart Show resolved Hide resolved

brustolin reviewed Feb 8, 2022

View reviewed changes

dart/lib/src/sentry_tracer.dart Outdated Show resolved Hide resolved

marandaneto mentioned this pull request Feb 25, 2022

fix: Document span.finish optional endTimestamp arg getsentry/develop#508

Merged

fatihergin force-pushed the feat/allow-to-set-sentryspan-starttimestamp-explicitly branch from d0b0db3 to bce00c3 Compare March 6, 2022 23:41

marandaneto reviewed Mar 7, 2022

View reviewed changes

fatihergin force-pushed the feat/allow-to-set-sentryspan-starttimestamp-explicitly branch from bce00c3 to dc1422a Compare March 8, 2022 13:36

fatih ergin added 2 commits March 8, 2022 21:19

Allow to set startTimestamp explicitly for SentrySpan

3206bdc

Allow to set endTimestamp manually when finishing span

2b2fd7d

fatihergin force-pushed the feat/allow-to-set-sentryspan-starttimestamp-explicitly branch from dc1422a to 2b2fd7d Compare March 8, 2022 18:28

marandaneto approved these changes Mar 10, 2022

View reviewed changes

marandaneto merged commit 7bc6840 into getsentry:main Mar 10, 2022

Uh oh!

Allow to set startTimestamp & endTimestamp manually to SentrySpan #676

Allow to set startTimestamp & endTimestamp manually to SentrySpan #676

Uh oh!

Conversation

fatihergin commented Dec 22, 2021 • 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!

vaind commented Jan 3, 2022

Uh oh!

Uh oh!

codecov-commenter commented Jan 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marandaneto Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

fatihergin Mar 6, 2022

Choose a reason for hiding this comment

Uh oh!

marandaneto Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

fatihergin Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marandaneto commented Feb 7, 2022

Uh oh!

marandaneto commented Feb 7, 2022

Uh oh!

marandaneto commented Feb 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fatihergin commented Feb 7, 2022

Uh oh!

marandaneto commented Feb 7, 2022

Uh oh!

Uh oh!

ueman commented Feb 7, 2022

Uh oh!

marandaneto commented Feb 7, 2022

Uh oh!

Uh oh!

marandaneto commented Mar 3, 2022

Uh oh!

marandaneto Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

marandaneto commented Mar 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fatihergin commented Dec 22, 2021 •

edited

Loading

codecov-commenter commented Jan 3, 2022 •

edited

Loading

fatihergin Mar 7, 2022 •

edited

Loading

marandaneto Mar 8, 2022 •

edited

Loading