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

[frontend] fix http.status_code on error #810

Merged
merged 4 commits into from
Mar 30, 2023
Merged

[frontend] fix http.status_code on error #810

merged 4 commits into from
Mar 30, 2023

Conversation

puckpuck
Copy link
Contributor

Fixes: #749

The http.status_code for synthetic requests to the frontend, was being set at span creation time. This changes that to set it just before the span ends. Looks like the app.frontend.requests metric was also affected by this issue.

Also removed some duplicated error handling code.

Signed-off-by: Pierre Tessier <[email protected]>
@puckpuck puckpuck requested a review from a team March 26, 2023 03:35
@cartersocha cartersocha requested a review from martinkuba March 26, 2023 04:09
@julianocosta89
Copy link
Member

When testing it I started getting some errors in the logs like this:

frontend                 | Can not execute the operation on ended Span {traceId: 9a72857f7ceef2b0e3a7856e0856e4e7, spanId: 214218836dea381f}
frontend                 | You can only call end() on a span once.

Besides that, it looks good to me.

@puckpuck
Copy link
Contributor Author

When testing it I started getting some errors in the logs like this:

frontend                 | Can not execute the operation on ended Span {traceId: 9a72857f7ceef2b0e3a7856e0856e4e7, spanId: 214218836dea381f}
frontend                 | You can only call end() on a span once.

Besides that, it looks good to me.

I do not see that error at all. I remember seeing it in the past, but trying again from main and this PR's branch, I can't reproduce it.

@martinkuba
Copy link
Contributor

martinkuba commented Mar 27, 2023

I am seeing the Can not execute the operation on ended Span error as well. It looks like it is this line. It is happening on the main branch for me as well. I think it makes sense since the span is from the http instrumentation when the site is NOT running in synthetic mode. I am guessing we just never noticed this before? The fix would be to end the span manually only when it is also created in this same file.

@puckpuck
Copy link
Contributor Author

Oh, I see. It only happens when you access the app from a browser UI (non-synthetic requests).

Yeah, I can plug in some logic to only close if we created it. Will update the PR later today.

Signed-off-by: Pierre Tessier <[email protected]>
@puckpuck
Copy link
Contributor Author

Updated the PR to only end the span when it's the one that we create to split synthetic requests.

Copy link
Member

@julianocosta89 julianocosta89 left a comment

Choose a reason for hiding this comment

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

Awesome! 🤩

@cartersocha
Copy link
Contributor

Can you add a changelog item please

@puckpuck puckpuck merged commit 954d35a into open-telemetry:main Mar 30, 2023
@puckpuck puckpuck deleted the frontend.fix_http_status_on_error branch March 30, 2023 00:06
JaredTan95 pushed a commit to openinsight-proj/opentelemetry-demo that referenced this pull request Apr 10, 2023
* Changelog entry for PR 797 (open-telemetry#803)

* Changelog entry for PR 797

* Changelog ordered

* lint fix

* Move Michael Maxwell to Emeritus (open-telemetry#800)

Co-authored-by: Carter Socha <[email protected]>
Co-authored-by: Juliano Costa <[email protected]>

* Bump actions/stale from 7 to 8 (open-telemetry#804)

Bumps [actions/stale](https://github.com/actions/stale) from 7 to 8.
- [Release notes](https://github.com/actions/stale/releases)
- [Changelog](https://github.com/actions/stale/blob/main/CHANGELOG.md)
- [Commits](actions/stale@v7...v8)

---
updated-dependencies:
- dependency-name: actions/stale
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* use absolute path (open-telemetry#806)

Signed-off-by: Pierre Tessier <[email protected]>

* use yamllint 1.3.0 (open-telemetry#807)

Signed-off-by: Pierre Tessier <[email protected]>

* [chore] add kubernetes manifest (open-telemetry#791)

* add kubernetes manifest

Signed-off-by: Pierre Tessier <[email protected]>

* add kubernetes manifest

Signed-off-by: Pierre Tessier <[email protected]>

* use absolute path

Signed-off-by: Pierre Tessier <[email protected]>

---------

Signed-off-by: Pierre Tessier <[email protected]>
Co-authored-by: Juliano Costa <[email protected]>

* Cart Service - minor cleanup (open-telemetry#801)

* Cart Service - minor cleanup

* fix file encoding

Signed-off-by: Pierre Tessier <[email protected]>

---------

Signed-off-by: Pierre Tessier <[email protected]>
Co-authored-by: Pierre Tessier <[email protected]>

* [frontend] update JS SDKs (open-telemetry#805)

* update JS SDKs

Signed-off-by: Pierre Tessier <[email protected]>

* update JS SDKs for frontend

Signed-off-by: Pierre Tessier <[email protected]>

* fix formatting

Signed-off-by: Pierre Tessier <[email protected]>

---------

Signed-off-by: Pierre Tessier <[email protected]>
Co-authored-by: Juliano Costa <[email protected]>
Co-authored-by: Austin Parker <[email protected]>

* Otlp env variables (open-telemetry#809)

* standardize OTEL_* env vars

Signed-off-by: Pierre Tessier <[email protected]>

* standardize OTEL_* env vars

Signed-off-by: Pierre Tessier <[email protected]>

---------

Signed-off-by: Pierre Tessier <[email protected]>
Co-authored-by: Juliano Costa <[email protected]>

* [frontend] fix http.status_code on error (open-telemetry#810)

* fix http.status_code on error

Signed-off-by: Pierre Tessier <[email protected]>

* only end span when synthetic

Signed-off-by: Pierre Tessier <[email protected]>

* fix http.status_code on error

Signed-off-by: Pierre Tessier <[email protected]>

---------

Signed-off-by: Pierre Tessier <[email protected]>

* Fix to shipping calculation (open-telemetry#814)

* reduce kafka mem allocation (open-telemetry#798)

* add kafka mem allocation to changelog (open-telemetry#817)

* Changed web tracer to use batch processor (open-telemetry#819)

* Updated ENV_PLATFORM flag (open-telemetry#818)

* Added elastic's forked opentelemetry demo repo (open-telemetry#813)

* Update collector (open-telemetry#822)

* use async php runtime (open-telemetry#823)

* use async php runtime
To better demonstrate PHP's capabilities, use an async runtime (react/http). This means that
batch exporters (traces and metrics) are long-lived and more efficient, and they can now use
export delays to only send batches after the configured time has elapsed.
Update auto-instrumentation extension to install from PECL (the preferred mechanism, which we've
just set up), and bump other dependencies to their latest beta versions.

* update changelog

* Add license check (open-telemetry#825)

* adding license check

* add/update copyrights

* add checklicense to gh checks

* add make target to add license

* fixup

* swap to short form license

* add copyright to yaml

* and the rest of the yaml

* fixup

* address comments

* fix prometheus scrape bug (open-telemetry#827)

Signed-off-by: Ziqi Zhao <[email protected]>
Co-authored-by: Austin Parker <[email protected]>

* prep for beta (open-telemetry#828)

---------

Signed-off-by: dependabot[bot] <[email protected]>
Signed-off-by: Pierre Tessier <[email protected]>
Signed-off-by: Ziqi Zhao <[email protected]>
Co-authored-by: Devrim Demiroz <[email protected]>
Co-authored-by: Reiley Yang <[email protected]>
Co-authored-by: Carter Socha <[email protected]>
Co-authored-by: Juliano Costa <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Pierre Tessier <[email protected]>
Co-authored-by: Austin Parker <[email protected]>
Co-authored-by: Martin Kuba <[email protected]>
Co-authored-by: Bahubali Shetti <[email protected]>
Co-authored-by: Brett McBride <[email protected]>
Co-authored-by: Ziqi Zhao <[email protected]>
juliangiuca pushed a commit to juliangiuca/opentelemetry-demo that referenced this pull request Apr 12, 2023
* fix http.status_code on error

Signed-off-by: Pierre Tessier <[email protected]>

* only end span when synthetic

Signed-off-by: Pierre Tessier <[email protected]>

* fix http.status_code on error

Signed-off-by: Pierre Tessier <[email protected]>

---------

Signed-off-by: Pierre Tessier <[email protected]>
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 2024
* fix http.status_code on error

Signed-off-by: Pierre Tessier <[email protected]>

* only end span when synthetic

Signed-off-by: Pierre Tessier <[email protected]>

* fix http.status_code on error

Signed-off-by: Pierre Tessier <[email protected]>

---------

Signed-off-by: Pierre Tessier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants