fix(trace): show status of span in trace pannel

[Defman]

This pr fixes missing status code from spans in the trace panel.

[SpencerTorres]

Hey thanks for the contribution!

Can you find a source where this value comes from: STATUS_CODE_ERROR? I looked through the trace spec and the Go SDK, but can't seem to find an exact source for that string. I want to make sure this string isn't unique to a specific language SDK and will properly work for all external sources before they reach our ClickHouse exporter. Perhaps this can also be tied to a version as a value within otel.ts so that it's not hardcoded

Also what does 2 mean in the if/else? I would like to have source links or docs for these in case we need to make changes in the future

[Defman]

I don't think that its written in some doc or spec. However its how its defined inside the otel collector.

https://github.com/open-telemetry/opentelemetry-collector/blob/d2ed276a9215053790ddead2d6479a6beacd28a7/pdata/ptrace/status_code.go#L14

[Defman]

Regarding the trace panel, the docs are more or less non existing.

However looking at the following source references

• https://github.com/grafana/grafana/blob/d3c6e2b203f87d210cdbf346198d960ac00e72ff/public/app/features/explore/TraceView/components/TraceTimelineViewer/SpanBarRow.tsx#L448
• https://github.com/grafana/grafana/blob/d3c6e2b203f87d210cdbf346198d960ac00e72ff/public/app/features/explore/TraceView/components/TraceTimelineViewer/VirtualizedTraceView.tsx#L446
• https://github.com/grafana/grafana/blob/d3c6e2b203f87d210cdbf346198d960ac00e72ff/public/app/features/explore/TraceView/components/TraceTimelineViewer/utils.tsx#L76
• https://github.com/grafana/grafana/blob/d3c6e2b203f87d210cdbf346198d960ac00e72ff/public/app/features/explore/TraceView/components/TraceTimelineViewer/utils.tsx#L73

[SpencerTorres]

Appreciate the links to the trace panel, the 2 makes sense now.

I see the first otel link you sent has some constants, but even the String function returns Error and not STATUS_CODE_ERROR. Perhaps its in some proto file somewhere?

I just want to make sure we have a source for this string that we can track in the future:

if("StatusCode" = 'STATUS_CODE_ERROR', 2, 0) as statusCode

[Defman]

I think I found what you were looking for @SpencerTorres

• https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#set-status
• https://github.com/open-telemetry/opentelemetry-proto/blob/e6923b74ccb43c884308b589cc00f972c12b4055/opentelemetry/proto/trace/v1/trace.proto#L319

[SpencerTorres]

I found the real source here: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/04b3b9898b242c0b3b707bc043c025eb9f6f73ba/internal/coreinternal/traceutil/traceutil.go#L37-L47

This is what is used in the ClickHouse exporter. I'm not sure whether I like this being a String, or if it should be changed to store the integer value. It's LowCardinality so I don't expect it to affect performance too much either way. Maybe an enum would be the best for both?

Edit: After reading the PR linked in the source, it seems like our plugin is using the OLD behavior. We really should be storing Unset, Ok, and Error (see open-telemetry/opentelemetry-collector#6250).

We should consider both values in the condition check for this PR so that when its changed over there, it will support both. I'm not sure I want to put all of this in the SQL though. If we need to, we can modify this value either on the backend or in the frontend as a postprocessor for the data frame.

Let me know if you have any opinions on all of this. I will review this PR as is for now.

[SpencerTorres]

I have submitted a PR to upgrade these statuses to match the specification: open-telemetry/opentelemetry-collector-contrib#34799

It would be nice if the Grafana code would match on this too, but that's for another PR: https://github.com/grafana/grafana/blob/d3c6e2b203f87d210cdbf346198d960ac00e72ff/public/app/features/explore/TraceView/components/TraceTimelineViewer/utils.tsx#L73

[SpencerTorres]

The PR has been merged today, going forward we should match on Error for the error case.

[Defman]

I have now replaced STATUS_CODE_ERROR with Error.

On another note @SpencerTorres do you by any chance have experience with using hosted Clickhouse and their autoscaler? Having the issue that their autoscaler really does not like the otel workload and wants to scaleup no matter what.

[Defman]

Hmm latest backend tests failed with

        	Error Trace:	/home/runner/work/clickhouse-datasource/clickhouse-datasource/pkg/plugin/driver_test.go:216
        	            				/home/runner/work/clickhouse-datasource/clickhouse-datasource/pkg/plugin/driver_test.go:1123
        	Error:      	Received unexpected error:
        	            	code: 44, message: Cannot create column with type 'JSON' because experimental JSON type is not allowed. Set setting allow_experimental_json_type = 1 in order to allow it

[SpencerTorres]

Appreciate the changes! Will review now.

On another note @SpencerTorres do you by any chance have experience with using hosted Clickhouse and their autoscaler? Having the issue that their autoscaler really does not like the otel workload and wants to scaleup no matter what.

Are you talking about ClickHouse Cloud? We have an excellent support team who will be able to look into those scaling issues. You should be able to open a ticket in our console.

Also, the backend tests failing is a known issue, I think @bossinc will be able to workaround that if we need to merge.

[adamyeats]

CI issue should be fixed now, please rebase the branch from main to get things up and running again! 👍

[Defman]

Are you talking about ClickHouse Cloud? We have an excellent support team who will be able to look into those scaling issues. You should be able to open a ticket in our console.

Yes @SpencerTorres I'm indeed talking about Clickhouse cloud and have opend a support request, however they are not able to help in regards to the OTel workload. As it is now, regardless of what node size I pick it will scale up, even if there is currently no load issue in regards to ingesting traces and etc. Was just wondering if you had experience with the cloud offering with Otel workload.

Kind reards
Jacob

[SpencerTorres]

Yes @SpencerTorres I'm indeed talking about Clickhouse cloud and have opend a support request...

I would recommend continuing this discussion with the support team in that request, they will have better answers than I do in regards to scaling. I sent them this thread internally so they have more context. If you have any issues they will absolutely be able to sort it out

[SpencerTorres]

Overall looks good and is a great feature addition. Added a few notes.

Thanks for the contribution!

[Defman]

Would using if("StatusCode" = 'STATUS_CODE_ERROR' OR "StatusCode" = 'Error', 2, 0) as statusCode be sufficient for backwards compatibility, such that users do not have to backfill the trace table.

[SpencerTorres]

Would using if("StatusCode" = 'STATUS_CODE_ERROR' OR "StatusCode" = 'Error', 2, 0) as statusCode be sufficient for backwards compatibility, such that users do not have to backfill the trace table.

I would prefer a single condition, but this option does offer the best compatibility. Yes, let's do it this way for now

[Defman]

@SpencerTorres I have now added the backwards compatible if statement.

[SpencerTorres]

@SpencerTorres I have now added the backwards compatible if statement.

It looks like this was added in the README but not the actual sqlgen code

[SpencerTorres]

Awesome, the code looks good now. Let's add a line to the CHANGELOG and then we can merge.

Appreciate your patience and contribution 🚀

[aangelisc]

Thank you @Defman!