Improve telemetry instrumentation on core flows #581
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
wcalderipe
force-pushed
the
chore/improve-instrumentation-on-core-flows
branch
from
b15771e
to
dc1ca85
Compare
There was a problem hiding this comment.
That looks super nice. Seem easy to setup new metrics from there. Appreciated to take a look on how you tested these metric and trace services with the stateful ones. I left a comment about one naming and one question.
Ty for the code comments from earlier PR with links to documentation, it was helpful.
| @@ -49,8 +55,6 @@ export const DEFAULT_HTTP_MODULE_PROVIDERS = [ | |||
| // Headers | |||
| // | |||
|
|
|||
| export const REQUEST_HEADER_CLIENT_ID = 'x-client-id' | |||
| export const REQUEST_HEADER_CLIENT_SECRET = 'x-client-secret' | |||
| export const REQUEST_HEADER_API_KEY = 'x-api-key' | |||
There was a problem hiding this comment.
Out of curiosity, why don't we also move the API_KEY one to shared ? Is it just used in armory ?
There was a problem hiding this comment.
I felt that was specific to the Armory, so I left it there. In contrast, x-client are used everywhere.
| private logger: LoggerService, | ||
| @Inject(MetricService) private metricService: MetricService | ||
| ) { | ||
| this.createCounter = this.metricService.createCounter('authorization_request_create_count') |
There was a problem hiding this comment.
Maybe rename createCounter attribute to something different than metricService create counter method ? Not a big deal but the first few seconds I was confused
Also, it seems so straightforward to setup new metrics, gj
There was a problem hiding this comment.
The class name carries the context needed to understand what "create" means. However, when the class has more than one responsibility, like in the KeyGenerationService, a more verbose name makes sense.
| @@ -7,6 +7,8 @@ Policy decision point for fine-grained authorization in web3.0. | |||
| - [Open Policy Agent (OPA) binary version >= | |||
| 0.69](https://www.openpolicyagent.org/docs/latest/#1-download-opa) installed | |||
| and accessible in your `$PATH`. | |||
| - [Regal](https://github.com/StyraInc/regal?tab=readme-ov-file#getting-started) | |||
| for linting Rego code. | |||
There was a problem hiding this comment.
I guess I should have put that in my latest Regal version PR, ty for adding that
wcalderipe
force-pushed
the
chore/improve-instrumentation-on-core-flows
branch
from
d67abab
to
884e89f
Compare
This PR improves telemetry instrumentation on core flows as well as adds business metrics to answer a few questions:
Changelog
OTEL_ATTR_CLIENT_ID,REQUEST_HEADER_CLIENT_ID, andREQUEST_HEADER_CLIENT_SECRETtonestjs-sharedpackage.x-client-idto the active span.authtoarmory.