Skip to content
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.

Sign up for GitHub

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

fix: updates ValueRecorder to allow negative values #1373

Merged
merged 4 commits into from
Aug 31, 2020
+25 −141
Merged

fix: updates ValueRecorder to allow negative values #1373

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2347435
fix: updates ValueRecorder to allow negative values
michaelgoin Jul 31, 2020
3aebc6f
Merge branch 'master' into allow-value-recorder-negative-values
michaelgoin Aug 27, 2020
2c04e1d
style: lint:fix
michaelgoin Aug 27, 2020
d7057dd
Merge branch 'master' into allow-value-recorder-negative-values
obecny Aug 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions packages/opentelemetry-api/src/metrics/BoundInstrument.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@
* limitations under the License.
*/

import { CorrelationContext } from '../correlation_context/CorrelationContext';
import { SpanContext } from '../trace/span_context';

/** An Instrument for Counter Metric. */
export interface BoundCounter {
/**
Expand All @@ -31,18 +28,8 @@ export interface BoundValueRecorder {
/**
* Records the given value to this value recorder.
* @param value to record.
* @param correlationContext the correlationContext associated with the
* values.
* @param spanContext the {@link SpanContext} that identifies the {@link Span}
* which the values are associated with.
*/
record(value: number): void;
record(value: number, correlationContext: CorrelationContext): void;
record(
value: number,
correlationContext: CorrelationContext,
spanContext: SpanContext
): void;
Comment on lines -40 to -45
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These signatures appeared completely unused. When get to the BoundValueRecorder they were just ignored, so I removed them. Have an open question in Gitter but figured I'd start with removing and can always add back if necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe these signatures came from the spec, but the spec on what to do with the correlations/span context was never finished.

Copy link
Contributor Author

@michaelgoin michaelgoin Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, interesting. Feels like these should be left out until that is known but obviously I am new to the project and may not understand all the impacts.

I couldn't find any current usages. May be some usages in the wild outside of these repos (js/contrib) but we are also < 1.0 so figured was OK to have a breaking change to clean-up at this point.

}

/** An Instrument for Base Observer */
Expand Down
21 changes: 0 additions & 21 deletions packages/opentelemetry-api/src/metrics/Metric.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@
* limitations under the License.
*/

import { CorrelationContext } from '../correlation_context/CorrelationContext';
import { SpanContext } from '../trace/span_context';
import {
BoundBaseObserver,
BoundCounter,
Expand Down Expand Up @@ -51,12 +49,6 @@ export interface MetricOptions {
*/
disabled?: boolean;

/**
* (Measure only, default true) Asserts that this metric will only accept
* non-negative values (e.g. disk usage).
*/
absolute?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValueRecorder was the only thing using absolute that I could find. Via #1241 it seems like that should not be configurable. As such, seemed natural to rip out while in here.


/**
* Indicates the type of the recorded value.
* @default {@link ValueType.DOUBLE}
Expand Down Expand Up @@ -148,19 +140,6 @@ export interface ValueRecorder extends UnboundMetric<BoundValueRecorder> {
* Records the given value to this value recorder.
*/
record(value: number, labels?: Labels): void;

record(
value: number,
labels: Labels,
correlationContext: CorrelationContext
): void;

record(
value: number,
labels: Labels,
correlationContext: CorrelationContext,
spanContext: SpanContext
): void;
}

/** Base interface for the Observer metrics. */
Expand Down
15 changes: 2 additions & 13 deletions packages/opentelemetry-api/src/metrics/NoopMeter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,19 +140,8 @@ export class NoopCounterMetric extends NoopMetric<BoundCounter>

export class NoopValueRecorderMetric extends NoopMetric<BoundValueRecorder>
implements ValueRecorder {
record(
value: number,
labels: Labels,
correlationContext?: CorrelationContext,
spanContext?: SpanContext
) {
if (typeof correlationContext === 'undefined') {
this.bind(labels).record(value);
} else if (typeof spanContext === 'undefined') {
this.bind(labels).record(value, correlationContext);
} else {
this.bind(labels).record(value, correlationContext, spanContext);
}
record(value: number, labels: Labels) {
this.bind(labels).record(value);
}
}

Expand Down
22 changes: 3 additions & 19 deletions packages/opentelemetry-metrics/src/BoundInstrument.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,42 +115,26 @@ export class BoundUpDownCounter extends BaseBoundInstrument
*/
export class BoundValueRecorder extends BaseBoundInstrument
implements api.BoundValueRecorder {
private readonly _absolute: boolean;

constructor(
labels: api.Labels,
disabled: boolean,
absolute: boolean,
valueType: api.ValueType,
logger: api.Logger,
aggregator: Aggregator
) {
super(labels, logger, disabled, valueType, aggregator);
this._absolute = absolute;
}

record(
value: number,
correlationContext?: api.CorrelationContext,
spanContext?: api.SpanContext
): void {
if (this._absolute && value < 0) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the critical crux of the change.

this._logger.error(
`Absolute ValueRecorder cannot contain negative values for $${Object.values(
this._labels
)}`
);
return;
}

record(value: number): void {
this.update(value);
}
}

/**
* BoundObserver is an implementation of the {@link BoundObserver} interface.
*/
export class BoundObserver extends BaseBoundInstrument {
export class BoundObserver extends BaseBoundInstrument
implements api.BoundBaseObserver {
constructor(
labels: api.Labels,
disabled: boolean,
Expand Down
1 change: 0 additions & 1 deletion packages/opentelemetry-metrics/src/Meter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ export class Meter implements api.Meter {
const opt: api.MetricOptions = {
logger: this._logger,
...DEFAULT_METRIC_OPTIONS,
absolute: true, // value recorders are defined as absolute by default
...options,
};

Expand Down
6 changes: 1 addition & 5 deletions packages/opentelemetry-metrics/src/ValueRecorderMetric.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ import { Metric } from './Metric';
/** This is a SDK implementation of Value Recorder Metric. */
export class ValueRecorderMetric extends Metric<BoundValueRecorder>
implements api.ValueRecorder {
protected readonly _absolute: boolean;

constructor(
name: string,
options: api.MetricOptions,
Expand All @@ -41,14 +39,12 @@ export class ValueRecorderMetric extends Metric<BoundValueRecorder>
resource,
instrumentationLibrary
);

this._absolute = options.absolute !== undefined ? options.absolute : true; // Absolute default is true
}

protected _makeInstrument(labels: api.Labels): BoundValueRecorder {
return new BoundValueRecorder(
labels,
this._disabled,
this._absolute,
this._valueType,
this._logger,
this._batcher.aggregatorFor(this._descriptor)
Expand Down
1 change: 0 additions & 1 deletion packages/opentelemetry-metrics/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ export const DEFAULT_CONFIG = {
/** The default metric creation options value. */
export const DEFAULT_METRIC_OPTIONS = {
disabled: false,
absolute: false,
description: '',
unit: '1',
valueType: api.ValueType.DOUBLE,
Expand Down
86 changes: 18 additions & 68 deletions packages/opentelemetry-metrics/test/Meter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,31 +476,6 @@ describe('Meter', () => {
assert.ok(valueRecorder instanceof Metric);
});

it('should be absolute by default', () => {
const valueRecorder = meter.createValueRecorder('name', {
description: 'desc',
unit: '1',
disabled: false,
});
assert.strictEqual(
(valueRecorder as ValueRecorderMetric)['_absolute'],
true
);
});

it('should be able to set absolute to false', () => {
const valueRecorder = meter.createValueRecorder('name', {
description: 'desc',
unit: '1',
disabled: false,
absolute: false,
});
assert.strictEqual(
(valueRecorder as ValueRecorderMetric)['_absolute'],
false
);
});

it('should pipe through resource', async () => {
const valueRecorder = meter.createValueRecorder(
'name'
Expand Down Expand Up @@ -559,10 +534,12 @@ describe('Meter', () => {
assert.doesNotThrow(() => boundValueRecorder.record(10));
});

it('should not accept negative values by default', async () => {
const valueRecorder = meter.createValueRecorder('name');
it('should not set the instrument data when disabled', async () => {
const valueRecorder = meter.createValueRecorder('name', {
disabled: true,
}) as ValueRecorderMetric;
const boundValueRecorder = valueRecorder.bind(labels);
boundValueRecorder.record(-10);
boundValueRecorder.record(10);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This diff makes things confusing but that test case was not touched, its just a shifting of code.


await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();
Expand All @@ -578,57 +555,30 @@ describe('Meter', () => {
);
});

it('should not set the instrument data when disabled', async () => {
const valueRecorder = meter.createValueRecorder('name', {
disabled: true,
}) as ValueRecorderMetric;
it('should accept negative (and positive) values', async () => {
const valueRecorder = meter.createValueRecorder('name');
const boundValueRecorder = valueRecorder.bind(labels);
boundValueRecorder.record(10);
boundValueRecorder.record(-10);
boundValueRecorder.record(50);

await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();
assert.deepStrictEqual(
record1.aggregator.toPoint().value as Distribution,
{
count: 0,
last: 0,
max: -Infinity,
min: Infinity,
sum: 0,
count: 2,
last: 50,
max: 50,
min: -10,
sum: 40,
}
);
assert.ok(
hrTimeToNanoseconds(record1.aggregator.toPoint().timestamp) >
hrTimeToNanoseconds(performanceTimeOrigin)
);
});

it(
'should accept negative (and positive) values when absolute is set' +
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case was modified to not pass any value for 'absolute' and then use existing assertions to assert both positive and negative values are accepted.

' to false',
async () => {
const valueRecorder = meter.createValueRecorder('name', {
absolute: false,
});
const boundValueRecorder = valueRecorder.bind(labels);
boundValueRecorder.record(-10);
boundValueRecorder.record(50);

await meter.collect();
const [record1] = meter.getBatcher().checkPointSet();
assert.deepStrictEqual(
record1.aggregator.toPoint().value as Distribution,
{
count: 2,
last: 50,
max: 50,
min: -10,
sum: 40,
}
);
assert.ok(
hrTimeToNanoseconds(record1.aggregator.toPoint().timestamp) >
hrTimeToNanoseconds(performanceTimeOrigin)
);
}
);

it('should return same instrument on same label values', async () => {
const valueRecorder = meter.createValueRecorder(
'name'
Expand Down