Skip to content

Include attributes from spans in log attributes - #3

Draft
kylewlacy wants to merge 3 commits into
PoiScript:afrom
blockgencorp:include-span-attributes-in-logs
Draft

Include attributes from spans in log attributes#3
kylewlacy wants to merge 3 commits into
PoiScript:afrom
blockgencorp:include-span-attributes-in-logs

Conversation

@kylewlacy

Copy link
Copy Markdown
Contributor

This PR updates NewRelicLayer to include more context when sending logs to New Relic. Specifically, it goes through each span in the event's scope and copies the span's attributes into the log's attributes.

As an example use case: imagine your service makes a top-level request span each time an HTTP request gets handled (this is what most Tracing web framework integrations do). Let's say that this request span has an http.target attribute attached. This PR will attach this http.target attribute to any event emitted in the scope of that span. In New Relic, this means you could add a filter like http.target:"/api/hello-world" and get all logs generated from the /api/hello-world endpoint.

I made this change because this matches what our current logging infrastructure does, and we've found it helpful to have span attributes attached to logs in order to filter and browse logs more easily.

I went with a pretty straightforward implementation to get this working, but I'm not sure if it's a good idea to have this behavior enabled without an option to disable it. I'm open to suggestions to revise or improve this PR!

@PoiScript

Copy link
Copy Markdown
Owner

Thanks for your contribution! I have few question and suggestion:

  1. Do we need to copy the span attributes recursively from all ancestors? Or just the current span?

  2. I think it's better to implement it inside the on_close method, since user can alter the span attribute after firing events:

fn foo() {
    let span = tracing::info_span!(foo = "bar");
    let _enter = span.enter();
    tracing::info!("foo");
    span.record("foo", &"baz");
}
  1. I would prefer it to be an optional feature, so user can decide whether to opt out.

@kylewlacy

Copy link
Copy Markdown
Contributor Author
  1. Do we need to copy the span attributes recursively from all ancestors? Or just the current span?

For our use case, we definitely want attributes copied recursively like this. We have a lot of request-level attributes, which can be used for querying or showing logs in a table. Here's an example:

New Relic log table, with columns including "http.target", "http.status_code", and "message"

Basically, it's handy to have all the attributes in the chain when viewing or querying logs. (Luckily, when querying traces, you can query by any attribute of any trace, so having the full chain of attributes isn't as useful there).

  1. I think it's better to implement it inside the on_close method, since user can alter the span attribute after firing events

Hmm, I think that's an interesting idea. From an end-user's standpoint, that would probably be better. With the way logging has been implemented, it seems like it shouldn't be too hard to do it this way. I'll try to see if I can get this working when I have some time to revise this PR.

  1. I would prefer it to be an optional feature, so user can decide whether to opt out.

Also shouldn't be too hard, I'll also try to implement this when I get some time!

@kylewlacy
kylewlacy marked this pull request as draft March 4, 2022 21:49
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.

2 participants