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

Jump to bottom

Fixing Span status when exporting span #1751

Merged
merged 7 commits into from
Dec 17, 2020
Merged

Fixing Span status when exporting span #1751

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3a6a490
chore: fixing status code for span to align with latest version
obecny Dec 14, 2020
1c6efa2
chore: updating to proto ver. 0.6 for exporter-collector-grpc
obecny Dec 14, 2020
902b3cb
chore: updating to proto ver. 0.6 for exporter-collector-proto
obecny Dec 14, 2020
ad16f71
chore: fixing tests after update
obecny Dec 14, 2020
42d7276
chore: updating readme and docker container version
obecny Dec 14, 2020
5253a72
chore: comparing using enums
obecny Dec 15, 2020
45e763e
Merge branch 'master' into span_status
obecny Dec 16, 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
2 changes: 1 addition & 1 deletion examples/collector-exporter-node/docker/docker-compose.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ version: "3"
services:
# Collector
collector:
image: otel/opentelemetry-collector:0.13.0
image: otel/opentelemetry-collector:0.16.0
# image: otel/opentelemetry-collector:latest
command: ["--config=/conf/collector-config.yaml", "--log-level=DEBUG"]
volumes:
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-collector-grpc/protos
Open in desktop
Submodule protos updated 3 files
+8 −0 README.md
+81 −27 opentelemetry/proto/trace/v1/trace.proto
+6 −6 opentelemetry/proto/trace/v1/trace_config.proto
6 changes: 5 additions & 1 deletion packages/opentelemetry-exporter-collector-grpc/test/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,11 @@ export function ensureExportedSpanIsCorrect(
assert.strictEqual(span.droppedLinksCount, 0, 'droppedLinksCount is wrong');
assert.deepStrictEqual(
span.status,
{ code: 'STATUS_CODE_OK', message: '' },
{
code: 'STATUS_CODE_OK',
deprecatedCode: 'DEPRECATED_STATUS_CODE_OK',
message: '',
},
'status is wrong'
);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-collector-proto/protos
Open in desktop
Submodule protos updated 3 files
+8 −0 README.md
+81 −27 opentelemetry/proto/trace/v1/trace.proto
+6 −6 opentelemetry/proto/trace/v1/trace_config.proto
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-collector/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
[![devDependencies][devDependencies-image]][devDependencies-url]
[![Apache License][license-image]][license-image]

This module provides exporter for web and node to be used with [opentelemetry-collector][opentelemetry-collector-url] - last tested with version **0.12.0**.
This module provides exporter for web and node to be used with [opentelemetry-collector][opentelemetry-collector-url] - last tested with version **0.16.0**.

## Installation

Expand Down
37 changes: 36 additions & 1 deletion packages/opentelemetry-exporter-collector/src/transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import {
Attributes,
Link,
SpanKind,
Status,
StatusCode,
TimedEvent,
TraceState,
} from '@opentelemetry/api';
Expand Down Expand Up @@ -184,12 +186,45 @@ export function toCollectorSpan(
droppedAttributesCount: 0,
events: toCollectorEvents(span.events),
droppedEventsCount: 0,
status: span.status,
status: toCollectorStatus(span.status),
links: toCollectorLinks(span, useHex),
droppedLinksCount: 0,
};
}

/**
* Converts StatusCode
* @param code
*/
export function toCollectorCode(
code: StatusCode
): opentelemetryProto.trace.v1.StatusCode {
switch (code) {
case StatusCode.OK:
return opentelemetryProto.trace.v1.StatusCode.OK;
case StatusCode.UNSET:
return opentelemetryProto.trace.v1.StatusCode.UNSET;
default:
return opentelemetryProto.trace.v1.StatusCode.ERROR;
}
}

/**
* Converts status
* @param status
*/
export function toCollectorStatus(
status: Status
): opentelemetryProto.trace.v1.Status {
const spanStatus: opentelemetryProto.trace.v1.Status = {
code: toCollectorCode(status.code),
};
if (typeof status.message !== 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to directly test for undefined value instead of using typeof

if(status.message !== undefined)

Copy link
Member Author

Choose a reason for hiding this comment

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

not really, if message is set to anything I just pass it through, this is not validation but just checking if the message was set or not and then pass it through exactly as it was

Copy link
Member

Choose a reason for hiding this comment

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

Yes but to check it you are using typeof and then compare the result to a string 'undefined'.
What is the benefit of it over testing directly for equality to undefined?

spanStatus.message = status.message;
}
return spanStatus;
}

/**
* Converts resource
* @param resource
Expand Down
28 changes: 26 additions & 2 deletions packages/opentelemetry-exporter-collector/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/

import { SpanKind, Logger, Attributes } from '@opentelemetry/api';
import * as api from '@opentelemetry/api';

/* eslint-disable @typescript-eslint/no-namespace */
export namespace opentelemetryProto {
Expand Down Expand Up @@ -254,7 +253,32 @@ export namespace opentelemetryProto {
status?: Status;
}

export type Status = api.Status;
export interface Status {
/** The status code of this message. */
code: StatusCode;
/** A developer-facing error message. */
message?: string;
}

/**
* An enumeration of status codes.
* https://github.com/open-telemetry/opentelemetry-proto/blob/master/opentelemetry/proto/trace/v1/trace.proto#L304
*/
export enum StatusCode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this (and all enums) const?

Again for better minification as during "compiliation" to JS, typescript converts all usages to the numeric value only.

The downside is that if you have code to convert "string" names back to an the enum value its not possible without adding your own extra "lookups"

/**
* The default status.
*/
UNSET = 0,
/**
* The operation has been validated by an Application developer or
* Operator to have completed successfully.
*/
OK = 1,
/**
* The operation contains an error.
*/
ERROR = 2,
}

export interface TraceConfig {
constantSampler?: ConstantSampler | null;
Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-exporter-collector/test/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ export function ensureSpanIsCorrect(
assert.strictEqual(span.droppedLinksCount, 0, 'droppedLinksCount is wrong');
assert.deepStrictEqual(
span.status,
{ code: api.StatusCode.OK },
{ code: opentelemetryProto.trace.v1.StatusCode.OK },
'status is wrong'
);
}
Expand Down