From 1112a2c8ff26512b163cafb5f006bf93b96c5cf4 Mon Sep 17 00:00:00 2001 From: Jim Anderson Date: Thu, 26 Sep 2024 18:23:12 -0500 Subject: [PATCH] fix to send all telemetry attributes --- common.ts | 34 ++++++++++++++++++++++-------- credentials/credentials.ts | 26 ++++++++++++++--------- docs/opentelemetry.md | 2 +- telemetry/attributes.ts | 16 ++++++-------- telemetry/configuration.ts | 2 +- tests/telemetry/attributes.test.ts | 14 ++---------- 6 files changed, 52 insertions(+), 42 deletions(-) diff --git a/common.ts b/common.ts index 1c23123..9a626ab 100644 --- a/common.ts +++ b/common.ts @@ -142,6 +142,11 @@ function randomTime(loopCount: number, minWaitInMs: number): number { return Math.floor(Math.random() * (max - min) + min); //The maximum is exclusive and the minimum is inclusive } +export interface WrappedAxiosResponse { + response?: AxiosResponse; + retries: number; +} + export async function attemptHttpRequest( request: AxiosRequestConfig, config: { @@ -149,12 +154,16 @@ export async function attemptHttpRequest( minWaitInMs: number; }, axiosInstance: AxiosInstance, -): Promise | undefined> { +): Promise | undefined> { let iterationCount = 0; do { iterationCount++; try { - return await axiosInstance(request); + const response = await axiosInstance(request); + return { + response: response, + retries: iterationCount - 1, + }; } catch (err: any) { if (!isAxiosError(err)) { throw new FgaError(err); @@ -194,17 +203,19 @@ export const createRequestFunction = function (axiosArgs: RequestArgs, axiosInst const maxRetry:number = retryParams ? retryParams.maxRetry : 0; const minWaitInMs:number = retryParams ? retryParams.minWaitInMs : 0; - const start = Date.now(); + const start = performance.now(); return async (axios: AxiosInstance = axiosInstance) : PromiseResult => { await setBearerAuthToObject(axiosArgs.options.headers, credentials!); - const axiosRequestArgs = {...axiosArgs.options, url: configuration.getBasePath() + axiosArgs.url}; - const response = await attemptHttpRequest(axiosRequestArgs, { + const url = configuration.getBasePath() + axiosArgs.url; + + const axiosRequestArgs = {...axiosArgs.options, url: url}; + const wrappedResponse = await attemptHttpRequest(axiosRequestArgs, { maxRetry, minWaitInMs, }, axios); - + const response = wrappedResponse?.response; const data = typeof response?.data === "undefined" ? {} : response?.data; const result: CallResult = { ...data }; setNotEnumerableProperty(result, "$response", response); @@ -212,6 +223,10 @@ export const createRequestFunction = function (axiosArgs: RequestArgs, axiosInst let attributes: StringIndexable = {}; attributes = TelemetryAttributes.fromRequest({ + userAgent: configuration.baseOptions?.headers["User-Agent"], + httpMethod: axiosArgs.options?.method, + url, + resendCount: wrappedResponse?.retries, start: start, credentials: credentials, attributes: methodAttributes, @@ -219,11 +234,12 @@ export const createRequestFunction = function (axiosArgs: RequestArgs, axiosInst attributes = TelemetryAttributes.fromResponse({ response, - credentials, attributes, }); - if (configuration.telemetry?.metrics?.histogramQueryDuration) { + // only if hisogramQueryDuration set AND if response header contains fga-query-duration-ms + const serverRequestDuration = attributes[TelemetryAttribute.HttpServerRequestDuration]; + if (configuration.telemetry?.metrics?.histogramQueryDuration && typeof serverRequestDuration !== "undefined") { configuration.telemetry.recorder.histogram( TelemetryHistograms.queryDuration, parseInt(attributes[TelemetryAttribute.HttpServerRequestDuration] as string, 10), @@ -237,7 +253,7 @@ export const createRequestFunction = function (axiosArgs: RequestArgs, axiosInst if (configuration.telemetry?.metrics?.histogramRequestDuration) { configuration.telemetry.recorder.histogram( TelemetryHistograms.requestDuration, - Date.now() - start, + attributes[TelemetryAttribute.HttpClientRequestDuration], TelemetryAttributes.prepare( attributes, configuration.telemetry.metrics.histogramRequestDuration.attributes diff --git a/credentials/credentials.ts b/credentials/credentials.ts index 289a49a..3156aa0 100644 --- a/credentials/credentials.ts +++ b/credentials/credentials.ts @@ -26,11 +26,11 @@ export class Credentials { private accessToken?: string; private accessTokenExpiryDate?: Date; - public static init(configuration: { credentials: AuthCredentialsConfig, telemetry: TelemetryConfiguration }): Credentials { - return new Credentials(configuration.credentials, globalAxios, configuration.telemetry); + public static init(configuration: { credentials: AuthCredentialsConfig, telemetry: TelemetryConfiguration, baseOptions?: any }): Credentials { + return new Credentials(configuration.credentials, globalAxios, configuration.telemetry, configuration.baseOptions); } - public constructor(private authConfig: AuthCredentialsConfig, private axios: AxiosInstance = globalAxios, private telemetryConfig: TelemetryConfiguration) { + public constructor(private authConfig: AuthCredentialsConfig, private axios: AxiosInstance = globalAxios, private telemetryConfig: TelemetryConfiguration, private baseOptions?: any) { this.initConfig(); this.isValid(); } @@ -129,9 +129,10 @@ export class Credentials { */ private async refreshAccessToken() { const clientCredentials = (this.authConfig as { method: CredentialsMethod.ClientCredentials; config: ClientCredentialsConfig })?.config; + const url = `https://${clientCredentials.apiTokenIssuer}/oauth/token`; try { - const response = await attemptHttpRequest<{ + const wrappedResponse = await attemptHttpRequest<{ client_id: string, client_secret: string, audience: string, @@ -140,8 +141,8 @@ export class Credentials { access_token: string, expires_in: number, }>({ - url: `https://${clientCredentials.apiTokenIssuer}/oauth/token`, - method: "post", + url, + method: "POST", data: { client_id: clientCredentials.clientId, client_secret: clientCredentials.clientSecret, @@ -156,6 +157,7 @@ export class Credentials { minWaitInMs: 100, }, globalAxios); + const response = wrappedResponse?.response; if (response) { this.accessToken = response.data.access_token; this.accessTokenExpiryDate = new Date(Date.now() + response.data.expires_in * 1000); @@ -166,18 +168,22 @@ export class Credentials { let attributes = {}; attributes = TelemetryAttributes.fromRequest({ + userAgent: this.baseOptions?.headers["User-Agent"], + fgaMethod: "TokenExchange", + url, + resendCount: wrappedResponse?.retries, + httpMethod: "POST", credentials: clientCredentials, - // resendCount: 0, // TODO: implement resend count tracking, not available in the current context + start: performance.now(), attributes, }); attributes = TelemetryAttributes.fromResponse({ response, - credentials: clientCredentials, - attributes, + attributes, }); - attributes = TelemetryAttributes.prepare(attributes); + attributes = TelemetryAttributes.prepare(attributes, this.telemetryConfig.metrics?.counterCredentialsRequest?.attributes); this.telemetryConfig.recorder.counter(TelemetryCounters.credentialsRequest, 1, attributes); } diff --git a/docs/opentelemetry.md b/docs/opentelemetry.md index ee83c7c..c259fcf 100644 --- a/docs/opentelemetry.md +++ b/docs/opentelemetry.md @@ -33,7 +33,7 @@ In cases when metrics events are sent, they will not be viewable outside of infr | `url.full` | `string` | No | The full URL of the request | | `url.scheme` | `string` | No | HTTP Scheme of the request (http/https) | | `http.request.resend_count` | `int` | Yes | The number of retries attempted | -| `http.client.request.duration` | `int` | No | Time taken by the FGA server to process and evaluate the request, in milliseconds | +| `http.client.request.duration` | `int` | No | Time taken by the FGA server to process and evaluate the request, rounded to the nearest milliseconds | | `http.server.request.duration` | `int` | No | The number of retries attempted | ## Default attributes diff --git a/telemetry/attributes.ts b/telemetry/attributes.ts index cba972e..dcc2129 100644 --- a/telemetry/attributes.ts +++ b/telemetry/attributes.ts @@ -67,7 +67,7 @@ export class TelemetryAttributes { attributes[TelemetryAttribute.UrlFull] = url; } - if (start) attributes[TelemetryAttribute.HttpClientRequestDuration] = Date.now() - start; + if (start) attributes[TelemetryAttribute.HttpClientRequestDuration] = Math.round(performance.now() - start); if (resendCount) attributes[TelemetryAttribute.HttpRequestResendCount] = resendCount; if (credentials && credentials.method === "client_credentials") { attributes[TelemetryAttribute.FgaClientRequestClientId] = credentials.configuration.clientId; @@ -78,24 +78,22 @@ export class TelemetryAttributes { static fromResponse({ response, - credentials, attributes = {}, }: { response: any; - credentials?: any; attributes?: Record; }): Record { if (response?.status) attributes[TelemetryAttribute.HttpResponseStatusCode] = response.status; const responseHeaders = response?.headers || {}; const responseModelId = responseHeaders["openfga-authorization-model-id"]; - const responseQueryDuration = responseHeaders["fga-query-duration-ms"]; - - if (responseModelId) attributes[TelemetryAttribute.FgaClientResponseModelId] = responseModelId; - if (responseQueryDuration) attributes[TelemetryAttribute.HttpServerRequestDuration] = responseQueryDuration; + const responseQueryDuration = responseHeaders["fga-query-duration-ms"] ? parseInt(responseHeaders["fga-query-duration-ms"], 10) : undefined; - if (credentials && credentials.method === "client_credentials") { - attributes[TelemetryAttribute.FgaClientRequestClientId] = credentials.configuration.clientId; + if (responseModelId) { + attributes[TelemetryAttribute.FgaClientResponseModelId] = responseModelId; + } + if (typeof responseQueryDuration !== "undefined" && Number.isFinite(responseQueryDuration)) { + attributes[TelemetryAttribute.HttpServerRequestDuration] = responseQueryDuration as number; } return attributes; diff --git a/telemetry/configuration.ts b/telemetry/configuration.ts index 9cb1767..505bf36 100644 --- a/telemetry/configuration.ts +++ b/telemetry/configuration.ts @@ -55,7 +55,7 @@ export class TelemetryConfiguration implements TelemetryConfig { // TelemetryAttribute.HttpRequestMethod, // TelemetryAttribute.UrlFull, // TelemetryAttribute.HttpClientRequestDuration, - // TelemetryAttribute.HttpServerRequestDuration + // TelemetryAttribute.HttpServerRequestDuration, // This not included by default as it has a very high cardinality which could increase costs for users // TelemetryAttribute.FgaClientUser diff --git a/tests/telemetry/attributes.test.ts b/tests/telemetry/attributes.test.ts index 87dd934..aa3559c 100644 --- a/tests/telemetry/attributes.test.ts +++ b/tests/telemetry/attributes.test.ts @@ -65,7 +65,7 @@ describe("TelemetryAttributes", () => { // Verify line 90 is covered - status is correctly set expect(result["http.response.status_code"]).toEqual(200); expect(result["fga-client.response.model_id"]).toEqual("model-id"); - expect(result["http.server.request.duration"]).toEqual("10"); + expect(result["http.server.request.duration"]).toEqual(10); }); test("should handle response without status correctly", () => { @@ -75,16 +75,6 @@ describe("TelemetryAttributes", () => { // Verify that no status code is set when response does not have a status expect(result["http.response.status_code"]).toBeUndefined(); expect(result["fga-client.response.model_id"]).toEqual("model-id"); - expect(result["http.server.request.duration"]).toEqual("10"); - }); - - test("should create attributes from response with client credentials", () => { - const response = { status: 200, headers: {} }; - const credentials = { method: "client_credentials", configuration: { clientId: "client-id" } }; - const result = TelemetryAttributes.fromResponse({ response, credentials }); - - // Check that the client ID is set correctly from the credentials - expect(result["http.response.status_code"]).toEqual(200); - expect(result["fga-client.request.client_id"]).toEqual("client-id"); + expect(result["http.server.request.duration"]).toEqual(10); }); });