-
Notifications
You must be signed in to change notification settings - Fork 3
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
Open Telemetry Integration #569
base: main
Are you sure you want to change the base?
Changes from 13 commits
d41b4cd
07fcb8d
25a43f9
39efad1
066b789
bc78505
73ce031
dbd5f44
f4e0664
b2fd806
a669155
da2edd4
43b821c
9d4e838
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,53 @@ | ||
import { MetricService, TraceService } from '@narval/nestjs-shared' | ||
import { PolicyStore } from '@narval/policy-engine-shared' | ||
import { HttpStatus, Injectable, NotFoundException } from '@nestjs/common' | ||
import { HttpStatus, Inject, Injectable, NotFoundException } from '@nestjs/common' | ||
import { Counter } from '@opentelemetry/api' | ||
import { ClientService } from '../../../client/core/service/client.service' | ||
import { ClusterService } from '../../../policy-engine/core/service/cluster.service' | ||
import { PolicyDataStoreRepository } from '../../persistence/repository/policy-data-store.repository' | ||
import { SignatureService } from './signature.service' | ||
|
||
@Injectable() | ||
export class PolicyDataStoreService extends SignatureService { | ||
private getCounter: Counter | ||
private setCounter: Counter | ||
|
||
constructor( | ||
private policyDataStoreRepository: PolicyDataStoreRepository, | ||
private clientService: ClientService, | ||
private clusterService: ClusterService | ||
private clusterService: ClusterService, | ||
@Inject(TraceService) private traceService: TraceService, | ||
@Inject(MetricService) private metricService: MetricService | ||
) { | ||
super() | ||
|
||
this.getCounter = this.metricService.createCounter('policy_data_store_get_count') | ||
this.setCounter = this.metricService.createCounter('policy_data_store_set_count') | ||
Comment on lines
+24
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we use constants for the metric names so that we have one place to look to find all the possible metrics we have? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would push the metrics discoverability problem to the observability stack rather than trying to solve it in the code. |
||
} | ||
|
||
async getPolicies(clientId: string): Promise<PolicyStore | null> { | ||
this.getCounter.add(1, { clientId }) | ||
|
||
const span = this.traceService.startSpan(`${PolicyDataStoreService.name}.getPolicies`, { | ||
attributes: { clientId } | ||
}) | ||
|
||
const policyStore = await this.policyDataStoreRepository.getLatestDataStore(clientId) | ||
|
||
return policyStore ? PolicyStore.parse(policyStore.data) : null | ||
const response = policyStore ? PolicyStore.parse(policyStore.data) : null | ||
|
||
span.end() | ||
|
||
return response | ||
} | ||
|
||
async setPolicies(clientId: string, payload: PolicyStore) { | ||
this.setCounter.add(1, { clientId }) | ||
|
||
const span = this.traceService.startSpan(`${PolicyDataStoreService.name}.setPolicies`, { | ||
attributes: { clientId } | ||
}) | ||
|
||
const client = await this.clientService.findById(clientId) | ||
|
||
if (!client) { | ||
|
@@ -46,10 +72,14 @@ export class PolicyDataStoreService extends SignatureService { | |
|
||
const success = await this.clusterService.sync(clientId) | ||
|
||
return { | ||
const response = { | ||
latestSync: { success }, | ||
policy: PolicyStore.parse(data), | ||
version | ||
} | ||
|
||
span.end() | ||
|
||
return response | ||
} | ||
} |
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'm questioning naming code -- is there a benefit to using
${EntityDataStoreService.name}
instead of just"EntityDataStoreService"
as a string? When using a raw string instead of interpolation, it makes it easier to do a "find all" in the codebase when you're looking for the line a log was from.Obv...it tells you the service & method name, so maybe my workflow is what doesn't make sense. But I tend to find something in logs, and want to just copy/paste in "find" to get to the file. wdyt?
I'm fine either way really, just throwing this out in case we all do this..
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.
The reason I opted for
${EntityDataStoreService.name}
is to make the span name refactoring proof. Although the side effects are:I'm on the fence now.. let me think a bit.