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.

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

Incorrect SpanKind sent by oltp-transformer #3631

Closed
gmreads opened this issue Feb 23, 2023 · 7 comments
Closed

Incorrect SpanKind sent by oltp-transformer #3631

gmreads opened this issue Feb 23, 2023 · 7 comments
Labels
bug Something isn't working triage

Comments

@gmreads
Copy link

gmreads commented Feb 23, 2023

What happened?

SpanKinds: https://github.com/open-telemetry/opentelemetry-js/blob/main/api/src/trace/span_kind.ts
1 -> server
2 -> client
ConsoleSpanProcessor logs span.kind=1 for server span.
But OLTP transformer increments it to 1 in function sdkSpanToOtlpSpan .

Steps to Reproduce

  • Send request to any instrumented service, with ConsoleSpanProcessor
    Span emitted by console processor.
{
  traceId: '9d9403c975afc9ee2432af980de7b2b8',
  parentId: undefined,
  traceState: undefined,
  name: 'HTTP POST',
  id: '661e557c1f2c1f7c',
  **kind: 1,** // NOTE here kind is server
  timestamp: 1677142257994000,
  duration: 51112,
  attributes: {
    'http.url': 'http://localhost:1337/post?xy=12',
    'http.host': 'localhost:1337',
    'net.host.name': 'localhost',
    'http.method': 'POST',
    'http.scheme': 'http',
    'http.target': '/post',

Expected Result

  • Data received by collector should be same as emitted by SpanProcessors
  • span.kind = 1 aka server
  • span.kind = SPAN_KIND_SERVER // This is also fine, numbers handling can be done internally by the sdk

Actual Result

  • OLTP transformer changes span kind to incorrect value
  • span.kind =2 // This changes the implication of span being server to client. Thus breaking the logic reliant on it.
{
	"resourceSpans": [{
			"resource": {
				"attributes": [{
						"key": "service.name",
						"value": {
							"stringValue": "unknown_service:node"
						}
				}],
			"droppedAttributesCount": 0
		},
		"scopeSpans": [{
			"scope": {
				"name": "@opentelemetry/instrumentation-http",
				"version": "0.35.1"
			},
			"spans": [{
				"traceId": "9d9403c975afc9ee2432af980de7b2b8",
				"spanId": "661e557c1f2c1f7c",
				"name": "HTTP POST",
				**"kind": 2,** // Transformer changes it to 2 incorrectly
				"attributes": [{
					"key": "http.url",
					"value": {
						"stringValue": "http://localhost:1337/post?xy=12"
					}
				}]
			}]
		}]
	}

Additional Details

Bug cause: https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/packages/otlp-transformer/src/trace/internal.ts#L38

span.kind: span.kind == null ? 0 : span.kind + 1

First added in PR: #2746

@dyladan I'de be happy to make a PR to fix this. Or is there some rationale behind the + 1 ?

OpenTelemetry Setup Code

No response

package.json

No response

Relevant log output

No response

@gmreads gmreads added bug Something isn't working triage labels Feb 23, 2023
@Flarna
Copy link
Member

Flarna commented Feb 23, 2023

numeric enum values in OTel-JS API are of by one compared to OTLP protocol, see here.

So OTEL export is correct.
For console exporter printing the enum string would be nice instead the numeric value (or both).

@gmreads
Copy link
Author

gmreads commented Feb 23, 2023

@Flarna Thanks, Is JS-API being off by one a permanent design decision or it can change in future ?

Nowhere in the chain we're using enum strings.
SpanProcessor -> Exporter -> Transformer(does off by one)-> Sends out numeric value

Using enum strings as span.kind, or span.kind.enum is this something can be merged upstream ?

Looks like for now I'll have to write my own exporter

@Flarna
Copy link
Member

Flarna commented Feb 23, 2023

The OTLP protocol and the OTel JS API enum values are two mostly independent types.
OTLP is just one exporter, there are others like Jaeger, Zipkin,... which may use their own type for SpanKind (and other enums).

Therefore noone should expect that numeric representation of enum values matches.

API users/instrumentations should use the enum type from API and don't care about the numeric value.
Similar receivers of OTLP should use the enum type from OTLP protocol and don't care if sender is Java, JS, Python,...

Using enum strings as span.kind, or span.kind.enum is this something can be merged upstream ?

Not sure if I understand you correct as usually the enum type is used not the numeric value, see e.g. here.

Using the calculated value in OTLP exporter is a shortcut/optimization to avoid a longer switch/case statement.

If your backend requires the SpanKind values from OTel JS I would recommend to fix this there and follow OTLP protocol instead JS API.
Using a customer exporter might work but be aware that you will need the same for Java, python,... as the exporters there use the same protocol as JS exporter does.

FWIW python seems to be also one of in their API see here.

@gmreads
Copy link
Author

gmreads commented Feb 23, 2023

@Flarna Yes, I agree we should use enum types from OLTP protocol not the numeric value.

But OLTP-exporter is not sending enum types back to the collector, instead it sends numeric values ex: span.kind=2 , which in case of Otel-JS refers to Server span.

In my receiver backend also, instead of receiving enum types, I am getting span.kind = 2. I expect it to either match with Otel API, or send an enum type.
Precisely to solve the problem you said, not caring about which language is sending this.

Thanks again.

@gmreads
Copy link
Author

gmreads commented Feb 23, 2023

@Flarna To be clear I want to propose this change in this sdkSpantoOltpSpan

Current
span.kind: span.kind == null ? 0: span.kind + 1

Proposed
span.kind: getEnumTypeFromNumber(span.kind == null ? 0: span.kind + 1)

@Flarna
Copy link
Member

Flarna commented Feb 23, 2023

Don't what you wan't to archive by this. OTLP is using protobuf and enums in protobuf are transferred as ints over the wire.

So everything else then a 2 on the wire for a server kind is simply wrong.

The conversion from the 2 one the wire to a nice enum value in your backend should be done by the protobuf lib of your choice.

@gmreads
Copy link
Author

gmreads commented Feb 24, 2023

Don't what you wan't to archive by this. OTLP is using protobuf and enums in protobuf are transferred as ints over the wire.

Oh! okay, I wasn't aware of this. Thanks, closing the issue then.

@gmreads gmreads closed this as completed Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage
Projects
None yet
Development

No branches or pull requests

2 participants