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

[TraceQL] Support ScopeSpans scope #2189

Open
joe-elliott opened this issue Mar 9, 2023 · 10 comments
Open

[TraceQL] Support ScopeSpans scope #2189

joe-elliott opened this issue Mar 9, 2023 · 10 comments
Assignees
Labels
keepalive Label to exempt Issues / PRs from stale workflow traceql

Comments

@joe-elliott
Copy link
Member

joe-elliott commented Mar 9, 2023

Is your feature request related to a problem? Please describe.
Since TraceQL was designed InstrumentationLibrarySpans was renamed ScopeSpans. Also attributes and intrinsics exist at this level.

Add support for querying these fields.

Before starting on this work we need to define how to address this particular scope. For instance traditionally intrinsics are accessed without scope:
{ name = "foo" }
and attributes are accessed with scope:
{ span.bar = "foo" }

We need to decide how to access intrinsics outside of the span scope. Consider

{ instrumentation.version = "1.0" }

to access the instrumentation's intrinsic version. How will we distinguish between this and an attribute named "version" at the instrumentation level? Should we keep with the pattern that intrinsics are unscoped and do something like?

{ instrumentation_version = "1.0" }

@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2023

This issue has been automatically marked as stale because it has not had any activity in the past 60 days.
The next time this stale check runs, the stale label will be removed if there is new activity. The issue will be closed after 15 days if there is no new activity.
Please apply keepalive label to exempt this Issue.

@github-actions github-actions bot added the stale Used for stale issues / PRs label May 9, 2023
@joe-elliott joe-elliott added keepalive Label to exempt Issues / PRs from stale workflow and removed stale Used for stale issues / PRs labels May 9, 2023
@mghildiy
Copy link
Contributor

Maybe I can have a go at it?

@mapno
Copy link
Member

mapno commented Jun 14, 2023

Go for it @mghildiy!

For the part of defining how to address those fields, take a look at this issue, where support for trace-level intrinsics is discussed. I think it can serve as inspiration.

@mghildiy
Copy link
Contributor

No new intrinsic fields to be added for this work. Going by OTL's InstrumentationScope, it has name and version as intrinsic properties and set of customizable attributes already.

Also, going through OTL's documentation, I see ScopeSpan has InstrumentationScope as a member. So objective of this work is to make traces searchable based on InstrumentationScope's name, version or any of its attribute.

@joe-elliott
Copy link
Member Author

No new intrinsic fields to be added for this work

I believe we need at least scopeName and scopeVersion? instrumentationScopeName? Oof that's a mouthful.

A good first step to implementing this feature will be adding support for scope attributes to the schema. We currently store the name/version but drop the attributes because they were added later.

type InstrumentationScope struct {
Name string `parquet:",snappy,dict"`
Version string `parquet:",snappy,dict"`
}

So for step 1, let's just add the attributes to the schema and correctly encode them here

https://github.com/grafana/tempo/blob/main/tempodb/encoding/vparquet2/schema.go#L243

and decode them here:

https://github.com/grafana/tempo/blob/main/tempodb/encoding/vparquet2/schema.go#L526

@mghildiy
Copy link
Contributor

mghildiy commented Jun 24, 2023

This would need modification in:

type InstrumentationScope struct {
// An empty instrumentation scope name means the name is unknown.
Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"`
Version string `protobuf:"bytes,2,opt,name=version,proto3" json:"version,omitempty"`
}

A new field to capture array of Attribute needs to be added.

@joe-elliott
Copy link
Member Author

So it has occurred to me that this will require a new Parquet version. This change can not be done in line to the existing vParquet2 encoding. Also, we already have a team working on vParquet3 with some nice perfomance improvements. I think this work is blocked until after vParquet3 is cut.

@Bruno-DaSilva
Copy link

Bruno-DaSilva commented Jul 13, 2023

@joe-elliott Does this issue include supporting searching other intrinsic fields, like ScopeSpan.Spans[x].Status.message or code? Or is this just for InstrumentationScope?

I'd like to +1 whatever issue makes status.message/code searchable :)

@joe-elliott
Copy link
Member Author

@Bruno-DaSilva If you're referring to the span status:

https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/trace/v1/trace.proto#L256

The code is already searchable:

{ status = ok }
{ status = error }
{ status = unset }

The message however is not. Created an issue to track:

#2655

@mdisibio
Copy link
Contributor

I'm not sure if this has been mentioned, but InstrumentationScope now contains arbitrary attributes. These need to be accommodated in both TraceQL and the new block format.

https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/common/v1/common.proto#L76C3-L79C36

message InstrumentationScope {
  // An empty instrumentation scope name means the name is unknown.
  string name = 1;
  string version = 2;

  // Additional attributes that describe the scope. [Optional].
  // Attribute keys MUST be unique (it is not allowed to have more than one
  // attribute with the same key).
  repeated KeyValue attributes = 3;
  uint32 dropped_attributes_count = 4;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keepalive Label to exempt Issues / PRs from stale workflow traceql
Projects
None yet
Development

No branches or pull requests

5 participants