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

Downgrade otelhttp to v0.37.0 #5515

Closed
wants to merge 2 commits into from
Closed

Conversation

youngbupark
Copy link

Description

There is the bug in otelhttp to use request.RemoteAddr for attributes, which results in high cardinality of the port of remoteaddr. This PR is to use the latest known good release.

open-telemetry/opentelemetry-go-contrib#3765

Issue reference

#5299
#5237

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Adds necessary unit tests for change
  • Adds necessary E2E tests for change
  • Unit tests passing
  • Extended the documentation / Created issue for it

@youngbupark youngbupark requested a review from a team as a code owner May 4, 2023 15:22
@github-actions
Copy link

github-actions bot commented May 4, 2023

Test Results

2 514 tests  ±0   2 507 ✔️ ±0   1m 57s ⏱️ +2s
   230 suites ±0          7 💤 ±0 
       1 files   ±0          0 ±0 

Results for commit 86a9f0b. ± Comparison against base commit 42a6f1b.

This pull request removes 2 and adds 2 tests. Note that renamed tests count towards both.
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/430687f4-6faf-424d-a547-95efbaa122ba
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/430687f4-6faf-424d-a547-95efbaa122ba#01
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/f800eef3-dfa1-4427-a163-e4dbbc7aaecd
github.com/project-radius/radius/pkg/armrpc/frontend/controller ‑ TestValidateEtag_IfMatch/f800eef3-dfa1-4427-a163-e4dbbc7aaecd#01

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented May 4, 2023

62.1

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

  • Your PR branch coverage: 62.1 %
  • main branch coverage: 62.1 %
  • diff coverage: 0 %

The coverage result does not include the functional test coverage.

@ytimocin
Copy link
Contributor

ytimocin commented May 4, 2023

Should we have an issue to track the work of upgrading back to the latest version once the otelhttp bug is fixed?

@youngbupark
Copy link
Author

Should we have an issue to track the work of upgrading back to the latest version once the otelhttp bug is fixed?

There is no benefit to upgrade pkg in the near future since their spec is constantly changing. As I left the comment here, it is better to wait until otelhttp has client side metric features. So I did not create new issue to upgrade it.

@github-actions
Copy link

github-actions bot commented May 4, 2023

62.1

For the detailed report, please go to Checks tab, click Build and Test, and then download unit_test_coverage artifact at the bottom of build page.

  • Your PR branch coverage: 62.1 %
  • main branch coverage: 62.1 %
  • diff coverage: 0 %

The coverage result does not include the functional test coverage.

@youngbupark
Copy link
Author

The old version misses status code attribute in http request which is required for our dashboard. So we decide to use the latest version with the workaround. In this case, we need to upgrade pkg as soon as the issue fixes. @vinayada1 will create the issue apart from https://github.com/project-radius/radius/issues/5308#issuecomment-1534984679

Closing this PR.

@youngbupark youngbupark closed this May 4, 2023
@youngbupark youngbupark deleted the youngp/downgrade-otel branch February 8, 2024 00:00
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.

2 participants