Skip to content

Commit e5e6635

Browse files
jimmyjamesjimmyjames
committed
updates from code review
1 parent 5d104a6 commit e5e6635

File tree

3 files changed

+45
-107
lines changed

3 files changed

+45
-107
lines changed

‎telemetry/configuration.ts

Lines changed: 21 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { TelemetryMetric, MetricRecorder } from "./metrics";
99
* @property {Set<TelemetryAttribute>} attributes - A set of attributes associated with the telemetry metric.
1010
*/
1111
export interface TelemetryMetricConfig {
12-
attributes: Set<TelemetryAttribute>;
12+
attributes?: Set<TelemetryAttribute>;
1313
}
1414

1515
/**
@@ -19,20 +19,7 @@ export interface TelemetryMetricConfig {
1919
* @property {Record<TelemetryMetric, TelemetryMetricConfig>} metrics - A record mapping telemetry metrics to their configurations.
2020
*/
2121
export interface TelemetryConfig {
22-
metrics: Partial<Record<TelemetryMetric, TelemetryMetricConfig>>;
23-
}
24-
25-
/**
26-
* Provides the configuration for a specific telemetry metric.
27-
*
28-
* @class TelemetryMetricConfiguration
29-
* @implements {TelemetryMetricConfig}
30-
* @param {Set<TelemetryAttribute>} [attributes=TelemetryConfiguration.defaultAttributes] - A set of attributes for the metric. Defaults to the standard attributes.
31-
*/
32-
export class TelemetryMetricConfiguration implements TelemetryMetricConfig {
33-
constructor(
34-
public attributes: Set<TelemetryAttribute> = TelemetryConfiguration.defaultAttributes
35-
) {}
22+
metrics?: Partial<Record<TelemetryMetric, TelemetryMetricConfig>>;
3623
}
3724

3825
/**
@@ -43,6 +30,8 @@ export class TelemetryMetricConfiguration implements TelemetryMetricConfig {
4330
*/
4431
export class TelemetryConfiguration implements TelemetryConfig {
4532

33+
public readonly recorder: MetricRecorder = new MetricRecorder();
34+
4635
/**
4736
* Default attributes for telemetry metrics.
4837
*
@@ -103,20 +92,21 @@ export class TelemetryConfiguration implements TelemetryConfig {
10392
* @param {Partial<Record<TelemetryMetric, TelemetryMetricConfig>>} [metrics] - A record mapping telemetry metrics to their configurations.
10493
*/
10594
constructor(
106-
public metrics: Partial<Record<TelemetryMetric, TelemetryMetricConfig>> = {},
107-
public recorder: MetricRecorder = new MetricRecorder(),
95+
public metrics?: Partial<Record<TelemetryMetric, TelemetryMetricConfig>>,
10896
) {
109-
const defaultMetrics: Record<TelemetryMetric, TelemetryMetricConfig> = {
110-
[TelemetryMetric.CounterCredentialsRequest]: new TelemetryMetricConfiguration(),
111-
[TelemetryMetric.HistogramRequestDuration]: new TelemetryMetricConfiguration(),
112-
[TelemetryMetric.HistogramQueryDuration]: new TelemetryMetricConfiguration(),
113-
};
114-
115-
// Merge provided metrics with default metrics
116-
this.metrics = {
117-
...defaultMetrics,
118-
...metrics,
119-
};
97+
if (!metrics) {
98+
this.metrics = {
99+
[TelemetryMetric.CounterCredentialsRequest]: {attributes: TelemetryConfiguration.defaultAttributes},
100+
[TelemetryMetric.HistogramRequestDuration]: {attributes: TelemetryConfiguration.defaultAttributes},
101+
[TelemetryMetric.HistogramQueryDuration]: {attributes: TelemetryConfiguration.defaultAttributes},
102+
};
103+
} else {
104+
this.metrics = {
105+
[TelemetryMetric.CounterCredentialsRequest]: metrics[TelemetryMetric.CounterCredentialsRequest] || undefined,
106+
[TelemetryMetric.HistogramRequestDuration]: metrics[TelemetryMetric.HistogramRequestDuration] || undefined,
107+
[TelemetryMetric.HistogramQueryDuration]: metrics[TelemetryMetric.HistogramQueryDuration] || undefined,
108+
};
109+
}
120110
}
121111

122112
/**
@@ -127,21 +117,21 @@ export class TelemetryConfiguration implements TelemetryConfig {
127117
public ensureValid(): void {
128118
const validAttrs = TelemetryConfiguration.validAttriburtes;
129119

130-
const counterConfigAttrs = this.metrics.counterCredentialsRequest?.attributes || new Set<TelemetryAttribute>();
120+
const counterConfigAttrs = this.metrics?.counterCredentialsRequest?.attributes || new Set<TelemetryAttribute>();
131121
counterConfigAttrs.forEach(counterConfigAttr => {
132122
if (!validAttrs.has(counterConfigAttr)) {
133123
throw new FgaValidationError(`Configuration.telemetry.metrics.counterCredentialsRequest attribute '${counterConfigAttr}' is not a valid attribute`);
134124
}
135125
});
136126

137-
const histogramRequestDurationConfigAttrs = this.metrics.histogramRequestDuration?.attributes || new Set<TelemetryAttribute>();
127+
const histogramRequestDurationConfigAttrs = this.metrics?.histogramRequestDuration?.attributes || new Set<TelemetryAttribute>();
138128
histogramRequestDurationConfigAttrs.forEach(histogramRequestDurationAttr => {
139129
if (!validAttrs.has(histogramRequestDurationAttr)) {
140130
throw new FgaValidationError(`Configuration.telemetry.metrics.histogramRequestDuration attribute '${histogramRequestDurationAttr}' is not a valid attribute`);
141131
}
142132
});
143133

144-
const histogramQueryDurationConfigAttrs = this.metrics.histogramQueryDuration?.attributes || new Set<TelemetryAttribute>();
134+
const histogramQueryDurationConfigAttrs = this.metrics?.histogramQueryDuration?.attributes || new Set<TelemetryAttribute>();
145135
histogramQueryDurationConfigAttrs.forEach(histogramQueryDurationConfigAttr => {
146136
if (!validAttrs.has(histogramQueryDurationConfigAttr)) {
147137
throw new FgaValidationError(`Configuration.telemetry.metrics.histogramQueryDuration attribute '${histogramQueryDurationConfigAttr}' is not a valid attribute`);

‎telemetry/metrics.ts

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,16 +15,6 @@ export class MetricRecorder {
1515
private _counters: Record<string, Counter> = {};
1616
private _histograms: Record<string, Histogram> = {};
1717

18-
constructor(
19-
meter?: Meter,
20-
counters?: Record<string, Counter>,
21-
histograms?: Record<string, Histogram>
22-
) {
23-
this._meter = meter || null;
24-
this._counters = counters || {};
25-
this._histograms = histograms || {};
26-
}
27-
2818
meter(): Meter {
2919
if (!this._meter) {
3020
this._meter = metrics.getMeter("@openfga/sdk", "0.6.3");

‎tests/telemetry/configuration.test.ts

Lines changed: 24 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1,87 +1,45 @@
1-
import { TelemetryMetricConfiguration, TelemetryConfiguration, TelemetryMetricConfig } from "../../telemetry/configuration";
1+
import { TelemetryConfiguration, TelemetryMetricConfig } from "../../telemetry/configuration";
22
import { TelemetryAttribute } from "../../telemetry/attributes";
33
import { TelemetryMetric } from "../../telemetry/metrics";
44

5-
describe("TelemetryMetricConfiguration", () => {
6-
test("should create a default TelemetryMetricConfiguration instance", () => {
7-
const config = new TelemetryMetricConfiguration();
8-
9-
expect(config.attributes.has(TelemetryAttribute.HttpHost));
10-
expect(config.attributes.has(TelemetryAttribute.HttpResponseStatusCode));
11-
expect(config.attributes.has(TelemetryAttribute.UserAgentOriginal));
12-
expect(config.attributes.has(TelemetryAttribute.HttpRequestMethod));
13-
expect(config.attributes.has(TelemetryAttribute.FgaClientRequestMethod));
14-
expect(config.attributes.has(TelemetryAttribute.FgaClientRequestClientId));
15-
expect(config.attributes.has(TelemetryAttribute.FgaClientRequestStoreId));
16-
expect(config.attributes.has(TelemetryAttribute.FgaClientRequestModelId));
17-
expect(config.attributes.has(TelemetryAttribute.HttpRequestResendCount));
18-
expect(config.attributes.has(TelemetryAttribute.FgaClientResponseModelId));
19-
20-
// should not be there
21-
expect(config.attributes.has(TelemetryAttribute.UrlScheme)).toBe(false);
22-
expect(config.attributes.has(TelemetryAttribute.HttpRequestMethod)).toBe(false);
23-
expect(config.attributes.has(TelemetryAttribute.UrlFull)).toBe(false);
24-
expect(config.attributes.has(TelemetryAttribute.FgaClientUser)).toBe(false);
25-
});
26-
27-
test("should return correct attributes based on enabled properties", () => {
28-
const config = new TelemetryMetricConfiguration(
29-
new Set<TelemetryAttribute>([
30-
TelemetryAttribute.FgaClientRequestClientId,
31-
TelemetryAttribute.HttpResponseStatusCode,
32-
TelemetryAttribute.UrlScheme,
33-
TelemetryAttribute.HttpRequestMethod,
34-
])
35-
);
36-
37-
const attributes = config.attributes;
38-
39-
expect(attributes.size).toBe(4);
40-
expect(attributes.has(TelemetryAttribute.FgaClientRequestClientId));
41-
expect(attributes.has(TelemetryAttribute.HttpResponseStatusCode));
42-
expect(attributes.has(TelemetryAttribute.UrlScheme));
43-
expect(attributes.has(TelemetryAttribute.HttpRequestMethod));
44-
});
45-
});
46-
475
describe("TelemetryConfiguration", () => {
48-
test("should create a default TelemetryConfiguration instance", () => {
49-
const config = new TelemetryConfiguration();
50-
51-
const counters = config.metrics.counterCredentialsRequest;
52-
expect(counters).toBeInstanceOf(TelemetryMetricConfiguration);
53-
expect(counters?.attributes).toEqual(TelemetryConfiguration.defaultAttributes);
54-
55-
const histogramQueryDuration = config.metrics.histogramQueryDuration;
56-
expect(histogramQueryDuration).toBeInstanceOf(TelemetryMetricConfiguration);
57-
expect(histogramQueryDuration?.attributes).toEqual(TelemetryConfiguration.defaultAttributes);
58-
59-
const histogramRequestDuration = config.metrics.histogramRequestDuration;
60-
expect(counters).toBeInstanceOf(TelemetryMetricConfiguration);
61-
expect(counters?.attributes).toEqual(TelemetryConfiguration.defaultAttributes);
62-
});
63-
646
test("should use defaults if not all metrics defined", () => {
657
const config = new TelemetryConfiguration({
66-
[TelemetryMetric.CounterCredentialsRequest]: new TelemetryMetricConfiguration(),
67-
[TelemetryMetric.HistogramQueryDuration]: new TelemetryMetricConfiguration(
68-
new Set<TelemetryAttribute>([
8+
[TelemetryMetric.CounterCredentialsRequest]: {},
9+
[TelemetryMetric.HistogramQueryDuration]: {
10+
attributes: new Set<TelemetryAttribute>([
6911
TelemetryAttribute.FgaClientRequestClientId,
7012
TelemetryAttribute.HttpResponseStatusCode,
7113
TelemetryAttribute.UrlScheme,
7214
TelemetryAttribute.HttpRequestMethod,
7315
])
74-
),
16+
}
7517
});
7618

77-
expect(config.metrics.counterCredentialsRequest?.attributes).toEqual(TelemetryConfiguration.defaultAttributes);
78-
expect(config.metrics.histogramQueryDuration?.attributes).toEqual(new Set<TelemetryAttribute>(
19+
expect(config.metrics?.counterCredentialsRequest?.attributes).toEqual(undefined);
20+
expect(config.metrics?.histogramQueryDuration?.attributes).toEqual(new Set<TelemetryAttribute>(
7921
new Set<TelemetryAttribute>([
8022
TelemetryAttribute.FgaClientRequestClientId,
8123
TelemetryAttribute.HttpResponseStatusCode,
8224
TelemetryAttribute.UrlScheme,
8325
TelemetryAttribute.HttpRequestMethod,
8426
])));
85-
expect(config.metrics.histogramRequestDuration?.attributes).toEqual(TelemetryConfiguration.defaultAttributes);
27+
expect(config.metrics?.histogramRequestDuration?.attributes).toEqual(undefined);
8628
});
29+
30+
test("should use defaults", () => {
31+
const config = new TelemetryConfiguration();
32+
33+
expect(config.metrics?.counterCredentialsRequest?.attributes).toEqual(TelemetryConfiguration.defaultAttributes);
34+
expect(config.metrics?.histogramQueryDuration?.attributes).toEqual(TelemetryConfiguration.defaultAttributes);
35+
expect(config.metrics?.histogramRequestDuration?.attributes).toEqual(TelemetryConfiguration.defaultAttributes);
36+
});
37+
38+
test("should be undefined if empty object passed", () => {
39+
const config = new TelemetryConfiguration({});
40+
41+
expect(config.metrics?.counterCredentialsRequest?.attributes).toEqual(undefined);
42+
expect(config.metrics?.histogramQueryDuration?.attributes).toEqual(undefined);
43+
expect(config.metrics?.histogramRequestDuration?.attributes).toEqual(undefined);
44+
});
8745
});

0 commit comments

Comments
 (0)