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

Remove log instrumentation #720

Closed
anuraaga opened this issue Jul 17, 2020 · 5 comments · Fixed by #803
Closed

Remove log instrumentation #720

anuraaga opened this issue Jul 17, 2020 · 5 comments · Fixed by #803

Comments

@anuraaga
Copy link
Contributor

anuraaga commented Jul 17, 2020

I randomly looked at the logging instrumentation and found experimentalLogCaptureThreshold. I'm not aware of anything close to this style of converting logs to ~0 duration spans in OpenTelemetry specs and suspect this is carried over from datadog. Does it make sense to remove this instrumentation to prevent confusion?

@trask
Copy link
Member

trask commented Jul 17, 2020

I added the logger instrumentations 😄. The are definitely out of spec. I can maintain them outside of this repo if no one else wants them.

@anuraaga
Copy link
Contributor Author

Ah if there's a clear owner I'm less concerned :) It seems like an interesting feature but should it be migrated to use events instead of creating new spans? (I'm guessing it comes pre-events). Having events for warning messages on my failed span seems quite cool!

FWIW, the reason I stumbled upon them is wanting to implement ID injection into logs. From what I recall, there was talk about speccing that out but that's been punted for now, but I think it's very important and want to implement it too so I may end up working in these modules too.

@trask
Copy link
Member

trask commented Jul 19, 2020

See #212 for why they are captured as spans for now instead of events.

ID injection into logs would be great, see #133 for some history on that. Agree this is an important feature that we can implement here even if not spec'd 👍

@anuraaga
Copy link
Contributor Author

Thanks for the link - yeah I sort of figured that's the reasoning. My thoughts are

  • Adding events to spans seems very enriching, especially for failure spans. I could see this becoming spec'd some day too as a poor man's alternative to ID correlation in the backend as it makes sense within the data model.

  • Sending logs as spans even without a currentspan seems like it's a replacement for a logs backend - in general don't most people have a logs backend before a tracing one? 😄 And because spans are a window of time, I see no chance of this becoming spec'd ever.

I couldn't tell from glancing at the instrumentation if it adds any overhead when the config flag disables all log capture, hopefully it doesn't.

Will leave it to you though on whether we switch these back to events or move them around or do anything at all :)

@trask
Copy link
Member

trask commented Jul 20, 2020

My needs match the logs vision otep (capturing logs both inside and outside of spans).

The logging instrumentation brings this functionality for me today, because I can map the spans back to our own logging data model in the exporter.

But I agree this is totally out-of-spec, and is going to cause confusion in this repo, so agree with removing it, and we can bring it back once we have a logging data model.

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 a pull request may close this issue.

3 participants