[WIP] feat: Add Heroku Log Drain Endpoint #5417
Conversation
AbhiPrasad
force-pushed
the
abhi-heroku-log-drain
branch
4 times, most recently
from
7e52c00 to
dcc11e8
Compare
AbhiPrasad
force-pushed
the
abhi-heroku-log-drain
branch
from
dcc11e8 to
fa715cd
Compare
| } | ||
|
|
||
| Ok(( | ||
| "", |
There was a problem hiding this comment.
Is this correct? The rsyslog crate seems to handle the remainders differently, e.g. in LineRaw.
| relay-otel = { workspace = true } | ||
| relay-protocol = { workspace = true } | ||
| relay-common = { workspace = true } | ||
| rsyslog = { workspace = true } |
There was a problem hiding this comment.
Crate doesn't seem to be adopted nor polished (e.g. docs), need to make sure it satisfies our requirements regarding panics etc.
On a first glance it seems good though.
| pub fn as_i64(&self) -> Option<i64> { | ||
| match self { | ||
| Value::I64(v) => Some(*v), | ||
| Value::U64(v) => Some(*v as i64), |
There was a problem hiding this comment.
This wraps from positive into negative, I think a fundamental method like this one should opt to return None instead .try_into().ok(). If you need wrapping behaviour I'd do it explicitly on the caller side.
|
|
||
| async fn handle( | ||
| content_type: RawContentType, | ||
| headers: HeaderMap, |
There was a problem hiding this comment.
That's fine, but I think it'd be nice to extract the specific headers with a custom type and FromRequestParts impl.
| /// Configures the [`Integration`] type with additional headers. | ||
| /// | ||
| /// Headers are stored in the item's `other` field and can be retrieved | ||
| /// during processing via `item.get_header()`. |
There was a problem hiding this comment.
| /// during processing via `item.get_header()`. | |
| /// during processing via [`Item::get_header`]. |
| let value = headers.get(header.http_header_name())?.to_str().ok()?; | ||
| Some((header.as_str().to_owned(), Value::String(value.to_owned()))) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
Untested, but with_type_and_headers takes IntoIterator, which means there shouldn't be a need to collect().
This is WIP
ref getsentry/sentry#91727
ref https://devcenter.heroku.com/articles/log-drains#https-drains
I still need to:
Do some more testing with the logplex parsingAdd an integration test (note that Heroku doesn't support attaching headers, so we need to use query param auth)If anyone wants to do any early reviews, feel free to take a look at
relay-ourlogs/src/heroku_to_sentry.rsandrelay-server/src/processing/logs/integrations/heroku.rs. That needs the most scrutiny.