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

Add otel instrumentation to http/grpc/sql libraries. #2440

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jeffmendoza
Copy link
Collaborator

Also add cli arg to the clis that use those libraries to enable exporting metrics and traces. Docs under pkg/metrics. Default to off and also change prometheus default to off.

Description of the PR

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If GraphQL schema is changed, GraphQL client updates/additions have been made
  • If OpenAPI spec is changed, make generate has been run
  • If ent schema is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

Also add cli arg to the clis that use those libraries to enable exporting
metrics and traces. Docs under pkg/metrics. Default to off and also change
prometheus default to off.

Signed-off-by: Jeff Mendoza <jlm@jlm.name>
@@ -117,6 +131,7 @@ you have access to read and write to the respective blob store.`,
}

initializeNATsandCollector(ctx, opts.pubsubAddr, opts.blobAddr, opts.publishToQueue)
shutdown(ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be via a defer as soon as we create it? Right now (at least for this function) it doesn't seem to make a difference but to future proof when we add some code path that returns early

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, in a number of cases (this one) we are not using special timing on shutdown and a defer would be better.

Other cases we should call shutdown when we get the C-c signal but before we call the context cancel function. I'll look through and see which should be change and which are more sensitive.

I think the main thing for the shutdown is to flush any metrics that are recorded but not sent (the send happens in batches). This will be more applicable in some of the guacone commands that use the instrumented clients but do no poll.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants