-
Notifications
You must be signed in to change notification settings - Fork 288
feat(o11y): add hibp request count and metrics endpoint #6365
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
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.
I didn't know how to test this myself, but the code changes look sensible.
One thing that might be relevant is that Next.js also has support for OpenTelemetry that can be enabled and automatically tags e.g. spans for endpoints. (This is me just linking concepts that I have vaguely heard about in my head, but I think I also heard something about not being able to use OTel yet?)
| if (globalThis.metrics !== undefined) return globalThis.metrics; | ||
| const registry = new client.Registry(); | ||
| client.collectDefaultMetrics({ register: registry }); |
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.
Just wondering if this shouldn't be done in instrumentation.ts. which I think is intended exactly for this use case?
(That would only work in the context of Next.js, but I imagine the setup is going to be different in different environments anyway.)
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.
Did you try the testing instructions?
Yes, I read through the documentation on otel. Unfortunately as you mentioned we can't use otel in our cluster because we don't have a collector which can receive the traces/spans. I didn't register the prometheus client in instrumentation.ts initially because it is node only, but I can follow the same instructions that they have in otel docs to import dynamically if we want to ensure it's registered at startup. |
ae71192 to
d4a5845
Compare
Do not attempt to make subscription outside of terraform Explicit error for nonexistent topic
1bc92f2 to
a1d4bd0
Compare
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.
This time I did follow the testing instructions, and I saw the error counter increment at /metrics. I did see the following errors in my console after the first call to /notify, and the MetadataLookupWarning every time — not sure if that's expected:
(node:36490) Warning: Accessing non-existent property 'Error' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)
(node:36490) MetadataLookupWarning: received unexpected error = All promises were rejected code = UNKNOWN
{"level":"error","message":"error_queuing_hibp_breach:","topicName":"hibp-breaches"}
POST /api/v1/hibp/notify 429 in 127508ms
|
|
||
| gcp: { | ||
| projectId: getEnvString("GCP_PUBSUB_PROJECT_ID", { | ||
| fallbackValue: isLocalOrTest ? "your-project-name" : 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 (unrelated to this PR): Hmm, maybe this approach is also something we could use to deal with env vars that we only need to be set at runtime, but not buildtime - only provide a fallback value if process.env.CI is set.
References:
Jira: MNTOR-5123
Figma:
Description
Add HIBP request counters (total, errors) and metrics endpoint for google-managed prometheus to scrape
Screenshot (if applicable)
Not applicable.
How to test
npm run dev