-
Notifications
You must be signed in to change notification settings - Fork 491
contrib/labstack/echo.v4: option to ignore based on the response #3363
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
Conversation
The existing `WithIgnoreRequest` [1] won't contain the response status code since it's checked before the underlying controller is executed [2]. Discussion DataDog#3341 [1] https://github.com/DataDog/dd-trace-go/blob/v1.72.2/contrib/labstack/echo.v4/option.go#L103-L109 [2] https://github.com/DataDog/dd-trace-go/blob/v1.72.2/contrib/labstack/echo.v4/echotrace.go#L53-L56
|
Hey @darccio 👋 Could you please have a look at this one? 🙏 |
mtoffl01
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rafaeljusto , thanks for your contribution! I left a small suggestion for improvement. Once addressed, we can look at merging this.
Applying suggestion from @mtoffl01 to improve code readability. DataDog#3363 (comment)
|
Hey @rafaeljusto , Thanks for making those changes! After taking a closer look, I realized I may have misunderstood your intention initially. I thought you were trying to modify the status code based on the response, but it looks like your goal is to conditionally drop the span depending on the response. Is that right? As it stands, your changes will start a span on line 71 that is never finished. That would leave the span open indefinitely, or until the process exits, rather than cleanly dropping it. I'm trying to better understand your motivation. Are you aiming to:
Can you share more about the use case behind this change? Understanding the problem you're solving will help me determine if this is the right approach. 🙇 |
Oh, that's not good. I don't want to cause any memory leak here. 😄 Thanks for the heads up! What would be the proper way to drop the span?
This one! I'm looking at dropping the entire trace when the response status code is 503 (service unavailable). I don't want to add |
|
Hey @rafaeljusto , Yes, you can configure sampling rules based on various criteria, including span tags — which would cover your status code criteria. Here's an example setup: Or via environment variable: The above configurations will drop all traces that contain a span with a 503 status code. It's a "head based sampling" mechanism; you can read more about head based sampling in tracer libraries here, if you like. Let me know how this sounds to you! |
|
Oh, the Closing this one. |
The existing
WithIgnoreRequestwon't contain the response status code since it's checked before the underlying controller is executed.Discussion #3341
What does this PR do?
Adds a new
WithIgnoreResponseoption to the Echo v4 middleware.Motivation
The existing
WithIgnoreRequestdoesn't have the response information when executed. This doesn't cover the scenario when you need to skip the trace based on a request path with a specific returning status code.Reviewer's Checklist
v2-devbranch and reviewed by @DataDog/apm-go.