-
Notifications
You must be signed in to change notification settings - Fork 20
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
Update gqlgen and otel dependencies #85
Conversation
Normally we'd wait for a release, but I don't know when the next one is due and master has a fix for upbound#78. Signed-off-by: Nic Cope <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this @negz! Left a nit and more of an open question below.
@@ -58,7 +58,7 @@ var ( | |||
metric.WithDescription("Total number of resolvers completed"), | |||
metric.WithUnit(unit.Dimensionless)) | |||
|
|||
resDuration = metric.Must(meter).NewInt64ValueRecorder("resolver.duration.ms", | |||
resDuration = metric.Must(meter).NewInt64Histogram("resolver.duration.ms", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@negz did you see a change in behavior switching to NewInt64Histogram
?
Based on:
- Write an example showing how to create a histogram from a value recorder open-telemetry/opentelemetry-go#1280
- and the linked issue in 1280 Buckets should be defined at instrument level open-telemetry/opentelemetry-go#689 (comment)
OpenTelemetry Metrics is explicitly moving away from instruments named for the action they
perform, and moving toward instruments named for how they are used. There is no Histogram
instrument, which is part of the reason buckets can't be configured at the instrument level.
In the forthcoming 0.4 metrics specification, there will be two instruments that will translate
into Prometheus histograms by default, those are going to be named ValueRecorder and
ValueObserver
It seems like we should be seeing histograms from using ValueRecorder
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tnthornton I didn't test for a behaviour change, but as far as I can tell this was just a rename in upstream. See for example open-telemetry/opentelemetry-go#2202 and specifically https://github.com/open-telemetry/opentelemetry-go/pull/2202/files#diff-06b5bbc47352183e5b82d7dfca06cafd665876b31dc9672bc7a99fa9ea4fdba7L78.
The otel metrics API has been a bit of a wild ride, but it seems like they settled on having a "Histogram" type after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. I just wanted to make sure we weren't going backwards 👍
...and refactor to accomodate the API changes we encounter every time we do. Signed-off-by: Nic Cope <[email protected]>
Signed-off-by: Nic Cope <[email protected]>
/backport Kiiiind of a breaking change in that it's a behaviour change, but it's one that fixes a bug. 🤔 |
Backport failed for Please cherry-pick the changes locally. git fetch origin release-0.1
git worktree add -d .worktree/backport-85-to-release-0.1 origin/release-0.1
cd .worktree/backport-85-to-release-0.1
git checkout -b backport-85-to-release-0.1
ancref=$(git merge-base 05daa524ed2d020a913a7641dcc558952ececd5a 826694132b655504f13dec810264a21cbd7688f6)
git cherry-pick -x $ancref..826694132b655504f13dec810264a21cbd7688f6 |
/backport Again! |
Backport failed for Please cherry-pick the changes locally. git fetch origin release-0.1
git worktree add -d .worktree/backport-85-to-release-0.1 origin/release-0.1
cd .worktree/backport-85-to-release-0.1
git checkout -b backport-85-to-release-0.1
ancref=$(git merge-base 05daa524ed2d020a913a7641dcc558952ececd5a 826694132b655504f13dec810264a21cbd7688f6)
git cherry-pick -x $ancref..826694132b655504f13dec810264a21cbd7688f6 |
/backport |
Backport failed for Please cherry-pick the changes locally. git fetch origin release-0.1
git worktree add -d .worktree/backport-85-to-release-0.1 origin/release-0.1
cd .worktree/backport-85-to-release-0.1
git checkout -b backport-85-to-release-0.1
ancref=$(git merge-base 05daa524ed2d020a913a7641dcc558952ececd5a 826694132b655504f13dec810264a21cbd7688f6)
git cherry-pick -x $ancref..826694132b655504f13dec810264a21cbd7688f6 |
Description of your changes
Fixes #78
The primary motivation for this PR is to fix the above issue. I'm tempted to wait for a release rather than pin master, but would like to at least confirm the fix works. It's also not clear to me what the new gqlgen release cadence will be, so it may be worth pinning to master rather than waiting for the next release.
I have:
make reviewable
to ensure this PR is ready for review.backport release-x.y
labels to auto-backport this PR, as appropriate.How has this code been tested
I've run this against Crossplane v1.5.0 and tested via the playground that it fixes #78. The following query:
Returns the below result:
I've also confirmed that (at a glance) the metrics I'd expect are showing up under
/metrics
.