feat(core): Deprecate span startTimestamp & endTimestamp
#10192
Conversation
| @@ -12,9 +12,14 @@ export function isMeasurementValue(value: unknown): value is number { | |||
| * Helper function to start child on transactions. This function will make sure that the transaction will | |||
| * use the start timestamp of the created child span if it is earlier than the transactions actual | |||
| * start timestamp. | |||
| * | |||
| * Note: this will not be possible anymore in v8, | |||
| * unless we do some special handling for browser here... | |||
| */ | |||
| export function _startChild(transaction: Transaction, { startTimestamp, ...ctx }: SpanContext): Span { | |||
There was a problem hiding this comment.
This is really the main place I found where we are overwriting the start time in a way that is not really compatible with the new model. How bad is it to drop this in v8, I wonder? 🤔 if we want to keep it we'll have to do some trickery to make this work here.
There was a problem hiding this comment.
This is very important because we use it to clamp the start timestamp of page load transactions to the browser native measurements for when the request started.
Do you have anything in mind on how we could still have this functionality?
There was a problem hiding this comment.
IMHO since this is a browser specific thing we'll probably use some internal/private API for this here... I don't think this is possible to do with OTEL native APIs. so probably we'll have to do something like this:
function clampSpanStartTime(span: Span, startTime: number) {
const existingStartTime = spanToJSON(span).start_timestamp;
if (!existingStartTime || existingStartTime > startTime) {
(span as BrowserInternalSpan).updateStartTime(startTime);
}
}or something along these lines, if you know what I mean..? Not pretty, but 🤷
size-limit report 📦
|
There was a problem hiding this comment.
Nice! As long as we find a way to somehow solve the IdleTransaction/Span use case for correcting the start time, I'm totally on board.
Random thought: If nothing else works, maybe we need to store the updated timestamp as an attribute and set the timestamp to this attribute value when processing 🤔
|
Ah, totally forgot to update MIGRATIONS.md, will do that!
This is generally OK because there we want to set the transaction end time to a specific timestamp based on the spans, which we can still do with these APIs. the tricky thing is updating the start time of the transaction retrospectively... |
Instead, you can use `spanToJSON()` to _read_ the timestamps. Updating timestamps after span creation will not be possible in v8. Also ensure that `span.end()` can only be called once.
mydea
force-pushed
the
fn/deprecate-span-timestamps
branch
from
49a6531
to
3c8260a
Compare
Instead, you can use
spanToJSON()to read the timestamps. Updating timestamps after span creation will not be possible in v8.Also ensure that
span.end()can only be called once - we already do that for transactions, and it saves us from checking e.g.span.endTimestampbefore callingspan.end(). This is also aligned with how OTEL does it (they emit a warning if trying to end a span twice).