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

Upgrade opentelemetry go #83

Merged
merged 2 commits into from
Mar 2, 2023
Merged

Upgrade opentelemetry go #83

merged 2 commits into from
Mar 2, 2023

Conversation

joshcarp
Copy link
Contributor

@joshcarp joshcarp commented Mar 2, 2023

Upgrades + code changes that are needed

Copy link

@pgmitche pgmitche left a comment

Choose a reason for hiding this comment

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

LGTM, had to check why the unit's changed, but its here:
open-telemetry/opentelemetry-go@fe6856e

@joshcarp joshcarp merged commit 2ef7009 into main Mar 2, 2023
@joshcarp joshcarp deleted the upgrade/opentelemetry branch March 2, 2023 18:36
@akshayjshah
Copy link
Member

LGTM, had to check why the unit's changed, but its here: open-telemetry/opentelemetry-go@fe6856e

This is - predictably - a tragically awful API.

@@ -35,6 +34,9 @@ const (
clientKey = "client"
requestKey = "request"
responseKey = "response"
dimensionless = "1"
bytes = "By"
milliseconds = "ms"
Copy link
Member

Choose a reason for hiding this comment

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

Post-hoc suggestion: these are pretty common names for this sort of code, which makes it likely that we'll mistakenly shadow them in a future change. Could we change to unitDimensionless, unitBytes, and unitMilliseconds instead?

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.

3 participants