-
Notifications
You must be signed in to change notification settings - Fork 291
feat(otel): opentelemetry instrumentation #6408
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
base: main
Are you sure you want to change the base?
Conversation
Local setup for opentelemetry collection, export, and dashboards. Sentry setup for opentelemetry compatibility Convert prom-client metrics to otel metrics for HIBP requests
Avoid double-counting db queries
128612b to
14c9c0a
Compare
c183730 to
9ecbac3
Compare
| * (e.g. used by OTEL_RESOURCE_ATTRIBUTES env var: | ||
| * 'k8s.container.name=monitor-api-endpoint,k8s.namespace.name=monitor-stage...') | ||
| * */ | ||
| export function parseKVList(str?: string): Record<string, string> { |
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.
nit: should this be in a util type folder?
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.
I co-located it where it was used like the other utility methods here (getEnvEnum, etc.). I don't think it is likely to be reused by anything else. Do you want them all moved somewhere?
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.
Any reason to not name this getEnvKvlist and then use that in config.otel.resourceAttributes directly, rather than turning that into a string and deferring parsing to the call site?
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.
good idea
| import * as Sentry from "@sentry/nextjs"; | ||
|
|
||
| export async function register() { | ||
| console.log( |
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.
nit: would there be any risk of this function getting invoked more than once on reload or how the runtime boots?
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.
good question, I will have to read next documentation
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.
Only once according to the docs https://nextjs.org/docs/app/api-reference/file-conventions/instrumentation#register-optional
| - Prometheus (scrapes metrics for grafana; in GCP environment we use Google-Managed Prometheus) | ||
| - Grafana (visualization) | ||
|
|
||
| To view metrics locally, visit [Grafana](http://localhost:3000/d/monitor-dashboard/monitor?orgId=1). Some default dashboard panels are seeded. To see traces, navigate to the [Explore] pane in Grafana and select the Tempo datasource. Note that the data won't propagate immediately, so wait a minute if you're not seeing expected activity show up. |
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.
praise: Thank you for this write-up! It was helpful in explaining the process and made the setup pretty painless.
Vinnl
left a comment
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.
It would probably be good if someone with more relevant experience took a look too, since I don't know half of these tools, and barely know the other half. So I mainly focused on running this myself.
Not everything worked for me, although I should also note that I'm using devenv instead of Docker, and thus had to align that with the Docker setup. (I did try Docker as well, but it was missing a dependency that I think gets installed when you run npm run dev (but when you ran it, it added the MacOS version) and didn't know how to add it.) I've opened a PR into this branch that adds some modifications that makes some values overridable to enable devenv use: #6440.
I do see data in the Grafana dashboard show up:
But if I go to the Explore page and select Tempo (which I've never heard of), as described in the readme, I see nothing:
Not sure what I'm doing wrong there...
| * (e.g. used by OTEL_RESOURCE_ATTRIBUTES env var: | ||
| * 'k8s.container.name=monitor-api-endpoint,k8s.namespace.name=monitor-stage...') | ||
| * */ | ||
| export function parseKVList(str?: string): Record<string, string> { |
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.
Any reason to not name this getEnvKvlist and then use that in config.otel.resourceAttributes directly, rather than turning that into a string and deferring parsing to the call site?
src/instrumentation.node.ts
Outdated
|
|
||
| declare global { | ||
| var metrics: Readonly<MetricsState>; | ||
| var __metrics: Readonly<MetricsState> | undefined; |
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.
Thought: Suddenly wondering if this is something for which Node's AsyncLocalContext would make sense? Though I suppose this works fine, so 🤷
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.
Can you explain more?
groovecoder
left a comment
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.
High-level review looks good. I didn't see metrics showing up in the pre-built monitor-dashboard in the local grafana, but I saw the data from Prometheus and Tempo show up so I assume it's working.
chore(instrumentation): use cached singleton vs. global metrics
3989c39 to
0326731
Compare
Description
Local setup for opentelemetry collection, export, and dashboards.
Sentry setup for opentelemetry compatibility
Convert prom-client metrics to otel metrics for HIBP requests
Example: query for a trace and failure metrics (dev environment)
How to test (local)
docker composenpm run devHow to test (dev env)
Via dev site
explorepane to search for tracesVia curl
hibp_notify_requests_total,hibp_notify_request_failures_total) (you will need to pull the dev hibp notify token in order to make successful requests)