-
Notifications
You must be signed in to change notification settings - Fork 94
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(spans): Ingest standalone spans #2620
Conversation
Here some OTLP spans I generated from a node-experimental project: {
"traceId": "89143b0763095bd9c9955e8175d1fb23",
"spanId": "e342abb1214ca181",
"parentSpanId": "0c7a7dea069bf5a6",
"name": "middleware - fastify -> @fastify/multipart",
"kind": 1,
"startTimeUnixNano": 1697620454980000000,
"endTimeUnixNano": 1697620454980078800,
"attributes": [
{
"key": "fastify.type",
"value": {
"stringValue": "middleware"
}
},
{
"key": "plugin.name",
"value": {
"stringValue": "fastify -> @fastify/multipart"
}
},
{
"key": "hook.name",
"value": {
"stringValue": "onResponse"
}
},
{
"key": "sentry.sample_rate",
"value": {
"intValue": 1
}
},
{
"key": "sentry.parentSampled",
"value": {
"boolValue": true
}
}
],
"droppedAttributesCount": 0,
"events": [],
"droppedEventsCount": 0,
"status": {
"code": 0
},
"links": [],
"droppedLinksCount": 0
} {
"traceId": "89143b0763095bd9c9955e8175d1fb23",
"spanId": "0c7a7dea069bf5a6",
"name": "GET /bottles",
"kind": 2,
"startTimeUnixNano": 1697620454925000000,
"endTimeUnixNano": 1697620454979637200,
"attributes": [
{
"key": "http.url",
"value": {
"stringValue": "http://localhost:4000/bottles"
}
},
{
"key": "http.host",
"value": {
"stringValue": "localhost:4000"
}
},
{
"key": "net.host.name",
"value": {
"stringValue": "localhost"
}
},
{
"key": "http.method",
"value": {
"stringValue": "GET"
}
},
{
"key": "http.scheme",
"value": {
"stringValue": "http"
}
},
{
"key": "http.target",
"value": {
"stringValue": "/bottles"
}
},
{
"key": "http.user_agent",
"value": {
"stringValue": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/117.0.0.0 Safari/537.36"
}
},
{
"key": "http.flavor",
"value": {
"stringValue": "1.1"
}
},
{
"key": "net.transport",
"value": {
"stringValue": "ip_tcp"
}
},
{
"key": "sentry.sample_rate",
"value": {
"intValue": 1
}
},
{
"key": "sentry.origin",
"value": {
"stringValue": "auto.http.otel.http"
}
},
{
"key": "net.host.ip",
"value": {
"stringValue": "::1"
}
},
{
"key": "net.host.port",
"value": {
"intValue": 4000
}
},
{
"key": "net.peer.ip",
"value": {
"stringValue": "::1"
}
},
{
"key": "net.peer.port",
"value": {
"intValue": 60502
}
},
{
"key": "http.status_code",
"value": {
"intValue": 500
}
},
{
"key": "http.status_text",
"value": {
"stringValue": "INTERNAL SERVER ERROR"
}
},
{
"key": "http.route",
"value": {
"stringValue": "/bottles"
}
}
],
"droppedAttributesCount": 0,
"events": [],
"droppedEventsCount": 0,
"status": {
"code": 2
},
"links": [],
"droppedLinksCount": 0
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! Long-term we'll need a rate limiting and dynamic sampling strategy for these spans, but as long as the ingestion's feature-flagged we can ship without it IMO.
Co-authored-by: Joris Bayer <[email protected]>
Run full normalization before extracting metrics from a standalone span. --------- Co-authored-by: Pierre Massat <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
exclusive_time | ||
.value() | ||
.ok_or(anyhow::anyhow!("missing exclusive_time"))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check means that we only accept spans for which the SDK sets exclusive_time
. So only from a Sentry SDK, not any other client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it so the fallback is to calculate the duration of the span using start and end timestamps. Like this, the validation still works as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment:
This is ultimately a product decision, but I don't think we should assume that a span has zero child spans just because the sentry-specific "exclusive time" attribute is missing. I'd rather drop these spans for now than ingest incorrect data for them.
), | ||
("http.request.method", "request.method"), | ||
("sentry.environment", "environment"), | ||
("sentry.exclusive_time_ns", "exclusive_time_ns"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Why is there a mapping for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it might be a good idea to have it but thinking about it more, it's useless, so I removed it.
Co-authored-by: Joris Bayer <[email protected]>
Co-authored-by: Joris Bayer <[email protected]>
}; | ||
if key == "exclusive_time_ns" { | ||
let value = match attribute.value { | ||
if key.contains("exclusive_time_ns") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is .contains
really what we want here? Don't we want to check for a specific key (or a set of keys)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can check for 2 specific keys indeed, I thought that'd be fine to just use contains
instead of repeating nearly exact key names.
if exclusive_time_ms == 0f64 { | ||
exclusive_time_ms = | ||
(from.end_time_unix_nano - from.start_time_unix_nano) as f64 / 1e6f64; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ultimately a product decision, but I don't think we should assume that a span has zero child spans just because the sentry-specific "exclusive time" attribute is missing. I'd rather drop these spans for now than ingest incorrect data for them.
if exclusive_time_ms == 0f64 { | |
exclusive_time_ms = | |
(from.end_time_unix_nano - from.start_time_unix_nano) as f64 / 1e6f64; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd rather keep them for now. Obviously, this is something that will change as we understand how to use this better from an OTLP point of view but I wouldn't reject them right now.
This is not meant to be used right away by customers and what we do with this field has to change before we release it but it's important to let us ingest some data, maybe a bit wrong at first, to continue testing.
exclusive_time | ||
.value() | ||
.ok_or(anyhow::anyhow!("missing exclusive_time"))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See other comment:
This is ultimately a product decision, but I don't think we should assume that a span has zero child spans just because the sentry-specific "exclusive time" attribute is missing. I'd rather drop these spans for now than ingest incorrect data for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have some reservations around the handling of "exclusive time", but other than that, this looks good to me!
I understand your reservations, I don't think we'll stick with this but it's important to get us started by not rejecting those spans until we have a solution. We'll likely have to talk to a few people to see what we'll need to do here. |
This PR aims to add support for ingesting standalone spans. This will support 3 ways to ingest spans:
Item
of typeOtelSpan
accepting an OpenTelemetry span, encoded as JSONItem
of typeSpan
accepting a span matching the span protocol, encoded as JSONAfter we accept those spans, they are converted to a span as the protocol describes it and they each go through the span processor which will:
They will then be sent to Kafka for storage.
In the end, we're aiming for this except our common format is the span protocol: