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

contrib/labstack/{echo, echo.v4}: improve error detection #1000

Merged
merged 7 commits into from Dec 9, 2022

Conversation

knusbaum
Copy link
Contributor

@knusbaum knusbaum commented Sep 8, 2021

This adds improved error detection to the echo integrations. Previously,
any returned error was recorded as an error in the echo span. This patch
causes the integration to inspect the returned error to determine if it is
a echo.HTTPError which it can use to discriminate between real errors (5xx
and up by default) or non-errors (below 5xx by default)

This also adds a WithStatusCheck Option which allows users to configure
which errors/statuses are reported as errors.

Fixes #987

@knusbaum knusbaum added this to the 1.34.0 milestone Sep 8, 2021
@knusbaum knusbaum requested a review from gbbr September 8, 2021 18:06
@knusbaum
Copy link
Contributor Author

knusbaum commented Sep 8, 2021

Manually tested, but need to add unit tests for this.

@knusbaum
Copy link
Contributor Author

knusbaum commented Sep 8, 2021

Done, ready.

@knusbaum knusbaum requested a review from gbbr September 21, 2021 20:00
@gbbr gbbr modified the milestones: 1.34.0, 1.35.0 Oct 20, 2021
@gbbr
Copy link
Contributor

gbbr commented Oct 20, 2021

Moving to 1.35.0. Hope that's ok. Otherwise ping me.

@knusbaum knusbaum requested a review from gbbr October 27, 2021 16:25
gbbr
gbbr previously approved these changes Oct 28, 2021
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

LGTM! Please mind the code freeze.

@sawadyecma
Copy link

sawadyecma commented Nov 26, 2021

LGTM 👍

I'm looking forward to merging.

Is there anything I can do ?

@felixge felixge modified the milestones: 1.35.0, 1.36.0 Dec 14, 2021
@knusbaum knusbaum requested review from a team and gbbr January 14, 2022 18:00
contrib/labstack/echo.v4/echotrace.go Outdated Show resolved Hide resolved
contrib/labstack/echo.v4/echotrace.go Outdated Show resolved Hide resolved
contrib/labstack/echo/echotrace.go Outdated Show resolved Hide resolved
This adds improved error detection to the echo integrations. Previously,
any returned error was recorded as an error in the echo span. This patch
causes the integration to inspect the returned error to determine if it is
a echo.HTTPError which it can use to discriminate between real errors (5xx
and up) or non-errors (4xx and below)

Fixes #987
@pr-commenter
Copy link

pr-commenter bot commented Dec 8, 2022

Benchmarks

Comparing candidate commit 649a37b in PR branch knusbaum/fix-echo with baseline commit 2f1555e in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 6 cases.

@knusbaum knusbaum modified the milestones: Triage, v1.45.0 Dec 8, 2022
Copy link
Contributor

@ajgajg1134 ajgajg1134 left a comment

Choose a reason for hiding this comment

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

Overall looks good, just two quick questions

contrib/labstack/echo.v4/echotrace.go Outdated Show resolved Hide resolved
contrib/labstack/echo.v4/echotrace_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ajgajg1134 ajgajg1134 left a comment

Choose a reason for hiding this comment

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

Looks good!

@knusbaum knusbaum merged commit 989e14a into main Dec 9, 2022
@knusbaum knusbaum deleted the knusbaum/fix-echo branch December 9, 2022 15:35
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.

Question: Datadog trace for echo is giving useless trace stack for 4xx and 5xx status code.
7 participants