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

otel: update breaking changing apis #323

Merged
merged 3 commits into from Oct 12, 2022
Merged

otel: update breaking changing apis #323

merged 3 commits into from Oct 12, 2022

Conversation

jsign
Copy link
Contributor

@jsign jsign commented Oct 3, 2022

Summary

This PR updates the OTEL dependencies, which have breaking APIs, and other deps.

Note: This PR will be a draft until open-telemetry/opentelemetry-go#3229 gets fixed in the next OTEL version. This is blocking because otelsql relies on OTEL deduping re-registering metrics; if that invariant doesn't hold, it causes a panic. We'll wait until there's a new OTEL version, and see how everything works.

After the problem above was fixed, there were two other breaking changes:

  • service_name and other satellite attributes aren't automatically added to metrics. This is now fixed with a metrics.BaseAttrs that should be used in other baseAttrs slices.
  • Under the new OTEL semantics, the "official" OTEL runtime package has a bug in defining multiple metrics as UpDownCounter instead of Gauge. That package has more logic than what we need, so I did a direct implementation of the metrics that interest us and removed the dependency,.

I also added an Uptime tile:
image

Memory metrics now show both api and healthbot series.

Context

We were notified by dependabot that new dependency versions were available.

Implementation overview

The OTEL breaking changes were a bit annoying since there's no documentation on how to fix them.
I had to do some GH search and code reading to figure out how to fix this properly.

Implementation details and review orientation

I slightly modified the bucket numbers to have extra buckets between the 10ms and 100ms range.

@jsign jsign added the on-hold label Oct 3, 2022
@jsign jsign self-assigned this Oct 3, 2022
@jsign jsign modified the milestone: 4.3.0 Oct 3, 2022
@jsign jsign added the chore label Oct 7, 2022
@jsign jsign force-pushed the jsign/updatedep2 branch 2 times, most recently from 1670583 to 96fe62a Compare October 12, 2022 13:37
@jsign jsign removed the on-hold label Oct 12, 2022
@jsign jsign added this to the 5.0.1 milestone Oct 12, 2022
@jsign jsign added enhancement New feature or request dependencies Pull requests that update a dependency file and removed chore labels Oct 12, 2022
@jsign jsign marked this pull request as ready for review October 12, 2022 13:57
@jsign jsign removed the request for review from asutula October 12, 2022 13:57
@jsign jsign marked this pull request as draft October 12, 2022 14:09
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
Signed-off-by: Ignacio Hagopian <jsign.uy@gmail.com>
@jsign jsign marked this pull request as ready for review October 12, 2022 14:52
Copy link
Collaborator

@brunocalza brunocalza left a comment

Choose a reason for hiding this comment

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

💯

@jsign jsign merged commit 42b3b3d into main Oct 12, 2022
@jsign jsign deleted the jsign/updatedep2 branch October 12, 2022 14:58
@jsign jsign mentioned this pull request Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants