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

Nginx: Improve tracing #283

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tiithansen
Copy link

@tiithansen tiithansen commented May 12, 2023

  1. Add possibility to use HTTP OTLP exporter.
  2. Improve tracing so that module creates internal or client span depending if the request is handled by Nginx directly or passed upstream.
  3. Set span status depending on result. If status code is between 400 - 599 set status as error.
  4. Catch internal processing failures in log phase.
  5. Rewrite tests in JS

Because of how Nginx works we can't determine what module will serve the actual request before its served. For what purpose I have implemented ProxyRecordable which makes it possible to change the span kind after processing in log phase.

1) Add possibility to use HTTP OTLP exporter.
2) Improve tracing so that module creates internal or client span depending if the request is handled by nginx directly or passed upstream.
3) Set span status depending on result. If status code is between 400 - 599 set status as error.
4) Catch internal processing failures in log phase.
5) Rewrite tests in JS
@tiithansen tiithansen requested a review from a team May 12, 2023 12:57
@tiithansen tiithansen changed the title Improve tracing Nginx: Improve tracing May 12, 2023
@esigo esigo self-assigned this May 14, 2023
@tiithansen
Copy link
Author

@esigo any plans with this one?

@vainikkaj
Copy link

I've got mixed feelings regarding span status. For example, it isn't always clear whether 4xx status codes are "errors". Does OTEL specification have clear stance on these?

Perhaps configuration flag for defining which status codes are viewed as span errors would be enough?

@huggsboson
Copy link

Just wanted to add an upvote on this one, I reached out to the nginx team and they were resistent to adding client spans to the instrumentation itself, so this seems like the best path forward:
nginxinc/nginx-otel#17

@huggsboson
Copy link

I've got mixed feelings regarding span status. For example, it isn't always clear whether 4xx status codes are "errors". Does OTEL specification have clear stance on these?

Perhaps configuration flag for defining which status codes are viewed as span errors would be enough?

I wouldn't consider 4xx's errors either, every time we did it in our other instrumentation it lead to a lot of false positives / noise.

@johanneswuerbach
Copy link
Contributor

@esigo / @seemk what would be required to move this forward?

@marcalff marcalff added the instrumentation:nginx Nginx module label Apr 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants