Skip to content
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

Natchez log4cats #36

Merged
merged 2 commits into from
Apr 9, 2020
Merged

Natchez log4cats #36

merged 2 commits into from
Apr 9, 2020

Conversation

tomverran
Copy link
Contributor

This is a small integration between Natchez & log4cats to allow us to pull headers from the Natchez kernel into the log4cats MDC so that in Datadog logs can be correlated with metrics

I didn't write any tests since this is so small, please forgive me!

@mwz
Copy link
Member

mwz commented Apr 9, 2020

Nice one!

I didn't write any tests since this is so small, please forgive me!

I'm not sure if I'll be able to forgive you that 😉

Comment on lines +22 to +25
(
headers.get("x-parent-id").map("dd.span_id" -> _) ++
headers.get("x-trace-id").map("dd.trace_id" -> _)
).toMap
Copy link
Member

Choose a reason for hiding this comment

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

I haven't checked the implementation yet, but I remember you were saying something that at the moment you can't add arbitrary tags into the kernel, but there is an open PR in natchez that allows you to do that? Did I get this right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is this: typelevel/natchez#18

For our specific needs I think the trace ID & span ID is enough for now as then Datadog is able to correlate logs to traces, and the traces then have all the other tags

Copy link
Member

@mwz mwz Apr 9, 2020

Choose a reason for hiding this comment

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

that makes sense 👍
I'm a little bit surprised because I thought that the "local" propagation was the default behaviour for any tags that you added to your parent span (and they would be propagated all the way down), but perhaps I misunderstood how it worked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the natchez-datadog module in this library does indeed do local propagation currently though I am wondering about changing it at some point so it is more consistent with the other Natchez backends - Since all the spans are shown together in a trace I think propagating tags locally probably isn't actually that useful nowadays

Copy link
Member

@mwz mwz Apr 9, 2020

Choose a reason for hiding this comment

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

So when the logs are correlated with a span, does it mean that you can then filter/search logs by the tags which are coming from the span? That's effectively the only useful purpose for local propagation from my perspective (so that I can search logs by tags), but if that already happens automatically in datadog, then it's probably not that useful to do the propagation like you're saying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think you can search logs with APM tags, unless you use natchez-slf4j which relays all the tracing information verbatim to SLF4J (while this module just correlates log4cats log lines with traces)

I think as we use Datadog more hopefully it'll become more obvious what we should do to make things as searchable as possible, I'll probably hold off making any changes until then

@mwz
Copy link
Member

mwz commented Apr 9, 2020

cc @ovotech/engagement-team

Copy link
Contributor

@ericaovo ericaovo left a comment

Choose a reason for hiding this comment

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

:fast_parrot:

@tomverran tomverran merged commit f6fae21 into master Apr 9, 2020
@tomverran tomverran deleted the natchez-log4cats branch April 9, 2020 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants