-
Notifications
You must be signed in to change notification settings - Fork 104
feat(ourlogs): Add OTLP endpoint for logs #5130
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
Conversation
jjbayer
left a comment
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.
Looks good AFAICT. A test case along the lines of
relay/tests/integration/test_nel.py
Line 8 in b044a69
| def test_nel_converted_to_logs(mini_sentry, relay): |
b044a69 to
9242f66
Compare
relay-server/src/envelope/item.rs
Outdated
| // NOTE: semantically wrong, but too expensive to parse. | ||
| ItemType::OtelLogsData => smallvec![ | ||
| (DataCategory::LogByte, self.len().max(1)), | ||
| (DataCategory::LogItem, item_count) |
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.
| // NOTE: semantically wrong, but too expensive to parse. | |
| ItemType::OtelLogsData => smallvec![ | |
| (DataCategory::LogByte, self.len().max(1)), | |
| (DataCategory::LogItem, item_count) | |
| ItemType::OtelLogsData => smallvec![ | |
| (DataCategory::LogByte, self.len().max(1)), | |
| (DataCategory::LogItem, item_count) // NOTE: semantically wrong, but too expensive to parse. | |
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.
Done with 68491f4
relay-server/src/endpoints/mod.rs
Outdated
| mod minidump; | ||
| mod monitor; | ||
| mod nel; | ||
| mod otel_log; |
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.
Should we rename this otlp_logs and rename traces to otlp_traces to match the endpoint routes?
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.
Agree, done with d30110c
| mod event; | ||
| mod metrics; | ||
| mod nel; | ||
| mod otel_log; |
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 would name the helper module log and rename the function to convert_otel_logs to be consistent with:
relay/relay-server/src/services/processor.rs
Line 2214 in ed78b72
| span::convert_otel_traces_data(managed_envelope); |
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 went with ourlog, done with cfe9765
| // Convert OTLP logs data to individual log items | ||
| otel_log::convert_to_logs(&mut managed_envelope); |
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.
Question for @Dav1dde for when you're back: Is this something that should move into LogsProcessor::prepare_envelope eventually?
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.
Done in #5136.
Maybe we want to refine this in the future, prepare_envelope was meant to be very very very lightweight, no processing at all. So it has to happen in the expansion. But this is good because we will apply everything we can before the expensive expansion (e.g. if we could inbound filters). It also means we don't have to deserialize -> serialize -> deserialize.
| if let Ok(()) = ItemContainer::from(logs).write_to(&mut item) { | ||
| envelope.envelope_mut().add_item(item); | ||
| } |
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 see this was copied from nel, but I would do at least .inspect_err() here. See
relay/relay-server/src/processing/spans/mod.rs
Lines 311 to 313 in ed78b72
| ItemContainer::from(self.spans) | |
| .write_to(&mut item) | |
| .inspect_err(|err| relay_log::error!("failed to serialize spans: {err}"))?; |
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 suggestion! I elected to pattern match and track a DiscardReason outcome. 334df53
relay-server/src/envelope/item.rs
Outdated
| ItemType::Span => true, | ||
| ItemType::OtelSpan => true, | ||
| ItemType::OtelTracesData => true, | ||
| ItemType::OtelLogsData => true, |
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 believe this should be false (also OtelTracesData in the line above). The otel types are already custom container types, so I don't believe they will ever be nested within an ItemContainer. cc @Dav1dde
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.
Yeah I had thought about this too, but I elected to follow the existing pattern with OtelTracesData in case there was some part of the processing pipeline that relied on this (or future work that was planned).
Changed with 8f451d7
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.
Yup correct, this should be false. The otel format is already a container 👍
jjbayer
left a comment
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.
Please keep us in the loop when you roll out the feature flag, & check if all the necessary ops changes were made (see traces).
…rocessor (#5136) The implementation in #5130 added the expansion right into the processor service, not the logs processor (e.g. how the old implementation did it #5082). This not only means we have to serialize and deserialize an additional time, it also goes against the design where we want to go towards. This was most likely caused by my 'NEL' hack, which I now also separated out again. We can implement a proper NEL processor once we pick up the NEL work again. The implementation also did not deal with outcomes correctly. This implementation fixes the issues and also introduces `RecordKeeper::modify_by` to track changes in outcome quantities exactly, instead of just `lenient`ly. We can also work towards moving all occurrences of `lenient` to `modify_by` in follow-up PRs.
This is a sanity check integration test to make sure that we are recording outcomes properly for otlp logs. The intial set of integration tests were added in #5130
resolves #5111
Building on top of #5128, this PR adds an OTLP endpoint for logs. Much of this approach was inspired by #4223.
To use this endpoint, you'll need to provide the following environmental variables (where
DSN_PROJECT_IDandDSN_PUBLIC_KEYare grabbed from the DSN) when setting up your OTEL setup. An integration test was added totests/integration/test_otlp_logs.pyto show this working.