Skip to content
This repository was archived by the owner on Aug 14, 2024. It is now read-only.

fix: Document span.finish optional endTimestamp arg #508

Merged
AbhiPrasad merged 1 commit into from
Feb 25, 2022
Merged

fix: Document span.finish optional endTimestamp arg #508

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
fix: Document span.finish optional endTimestamp arg 
The `span.finish()` and `transaction.finish()` methods can be given an optional `endTimestamp` value that allows for users to override the endTimestamp of a span/transaction. This is currently implemented in the JavaScript SDK

https://github.com/getsentry/sentry-javascript/blob/bb6f86502e1ea551fa503136a954844f77cca322/packages/tracing/src/span.ts#L232-L234

This PR updates the develop documentation to reflect this capability.
  • Loading branch information
@AbhiPrasad
AbhiPrasad authored Feb 24, 2022
commit 54086980b5d0ec120cfdd2a4afd025c91b93fab3
4 changes: 3 additions & 1 deletion src/docs/sdk/performance/index.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,13 @@ tree as well as the unit of reporting to Sentry.

- `Span.finish()`

- Just set `endTimestamp` to the current time (in payload `timestamp`)
- Accepts an optional `endTimestamp` to allow users to set a custom `endTimestamp` on the finished span
Copy link
Contributor

@marandaneto marandaneto Feb 25, 2022

Choose a reason for hiding this comment

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

Is there any sort of validation? e.g. if the given endTimestamp if before the startTimestamp and if so, should it log an error or what?
Asking that because of getsentry/sentry-dart#676

Copy link
Member Author

@AbhiPrasad AbhiPrasad Feb 25, 2022

Choose a reason for hiding this comment

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

At the current moment there is no validation in JS or Python. We can think about adding this though? Should we add to the spec?

Copy link
Contributor

@marandaneto marandaneto Feb 25, 2022

Choose a reason for hiding this comment

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

Ideally yes, our PR logs that the endTimestamp is before the startTimestamp and sets the current timestamp.
If this sounds like a good approach, we could document to the spec.

Copy link
Member Author

@AbhiPrasad AbhiPrasad Feb 25, 2022

Choose a reason for hiding this comment

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

I think we should ask around. Think logging is a good idea, but not sure about setting current timestamp if there is an error.

Copy link
Contributor

@marandaneto marandaneto Feb 25, 2022

Choose a reason for hiding this comment

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

This is in TSC already, we can make another PR if that's the case, so we unblock this one.

- If an `endTimestamp` value is not provided, set `endTimestamp` to the current time (in payload `timestamp`)

- `Transaction.finish()`
- `super.finish()` (call finish on Span)
- Send it to Sentry only if `sampled == true`
- Like spans, can be given an optional `endTimestamp` value that should be passed into the `span.finish()` call
- A `Transaction` needs to be wrapped in an `Envelope` and sent to the [Envelope Endpoint](/sdk/envelopes/)
- The `Transport` should use the same internal queue for `Transactions` / `Events`
- The `Transport` should implement category-based rate limiting →
Expand Down