feat(eap): Add a GetTraces endpoint #71

phacops · 2024-12-13T15:49:54Z

This will add Protobuf files describing a new endpoint to find traces. It will return a list of traces with fields requested.

Features:

You can pick which fields from the enum
You can specify the type you want from a field
You can use the same filter conditions as for the TraceItemTable endpoint
You can order by any field from the enum

Limitations:

No aggregation possible
No support for alias
A fixed list of fields to query

Since traces are not a defined object but a specific view on an aggregated list of spans, I don't think we should plan to support any field possible. Maybe at some point, we'll make the trace a real object and we'll add trace attributes and by then, we can design a new version of the endpoint.

Closes https://github.com/getsentry/eap-planning/issues/121.

proto/sentry_protos/snuba/v1/endpoint_get_traces.proto

colin-sentry · 2024-12-16T20:49:12Z

proto/sentry_protos/snuba/v1/endpoint_get_traces.proto

+import "sentry_protos/snuba/v1/trace_item_filter.proto";
+
+message TraceColumn {
+  enum Name {


just a few things:

rename some of these, I would move enum Name to top-level scope and call it something like enum TraceMetadataType or TraceKey or TraceAttributeKey

(subjective) rename Trace in response to just be Row

(subjective) the whole request might be better called 'TraceTableRequest' or something like that

add docstrings to at least the top-level messages, and each enum member

Why move the enum to top-level? It's really meant to be used only in this endpoint for a request, why not keep it embedded?

kylemumma

I've been going back through our proto definitions and adding better documentation on each of the Request and Response protos, so users can understand what the endpoints are and how to use them. I would appreciate if you could add these comments for documentation

Add a comment to the request object that describes the endpoint
add comments to each of the inputs in the request object to describe what it is (if non obvious)
add comments to each field of the response object to describe what it is (if non obvious)

volokluev · 2024-12-16T23:35:54Z

Can you add some example python files like we have for the other endpoints so we can see the API in action?

volokluev · 2024-12-16T23:44:17Z

proto/sentry_protos/snuba/v1/endpoint_get_traces.proto

+  }
+
+  RequestMeta meta = 1;
+  TraceItemFilter filter = 2;


this won't work for searching across event types. At least not by itself. The filter currently does not support a name. That's why in the other file there's extra filter definitions for traces (because they are different).

I didn't want to enable filtering on traces, I want to only allow filtering on trace items, that's why I removed it.

Traces don't have a name, they don't have attributes, I don't think we should enable filtering on things that are defined by trace item fields.

As for searching across event types, wouldn't it let me add a filter for a different even type than spans? Why is it preventing me from cross-event searches specifically? Maybe we'd need to allow a list of TraceItemFilter with an event type for each?

with your current API, how would you find all traces where span.op = http and error.project_id=1?

Pass a TraceItemFilter with your conditions, we map the prefixes to each dataset and we query.

I think we could do better by adding groups of filtered with a specific event type though.

we map the prefixes to each dataset and we query.

Can you elaborate?

or event better, show me an example with python

I think the latest commits should fix your concerns.

github-actions · 2024-12-18T15:30:40Z

The latest Buf updates on your PR. Results from workflow ci / buf-checks (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
`✅ passed`	`⏩ skipped`	`✅ passed`	`✅ passed`	Dec 18, 2024, 10:33 PM

proto/sentry_protos/snuba/v1/endpoint_get_traces.proto

Zylphrex · 2024-12-18T19:14:56Z

proto/sentry_protos/snuba/v1/endpoint_get_traces.proto

+  // List of attributes we want to query.
+  repeated TraceAttribute attributes = 6;


Does this mean we need to specify the type of every attribute when querying? A little inconvenient but not a deal breaker.

Fair enough, I'll remove it if you don't want it and document the type of each attribute in the docstring.

We'll likely want it in the response? So I'm okay with keeping it for now

I added it back.

proto/sentry_protos/snuba/v1/endpoint_get_traces.proto

… attributes

Zylphrex · 2024-12-18T19:42:30Z

proto/sentry_protos/snuba/v1/endpoint_get_traces.proto

+  // List of attributes we want to query.
+  repeated TraceAttribute attributes = 6;


We'll likely want it in the response? So I'm okay with keeping it for now

phacops added 2 commits December 13, 2024 10:38

feat(eap): Add a FindTraces endpoint

bcfc671

Add more possible fields

88ec482

phacops requested review from a team as code owners December 13, 2024 15:49

phacops mentioned this pull request Dec 13, 2024

feat(eap): Add a GetTraces endpoint getsentry/snuba#6671

Merged

phacops changed the title ~~Pierre/eap find traces~~ feat(eap): Add a FindTraces endpoint Dec 13, 2024

Move endpoint to a new file and revert changes in original file

bc55639

phacops changed the title ~~feat(eap): Add a FindTraces endpoint~~ feat(eap): Add a GetTraces endpoint Dec 13, 2024

colin-sentry reviewed Dec 16, 2024

View reviewed changes

proto/sentry_protos/snuba/v1/endpoint_get_traces.proto Outdated Show resolved Hide resolved

colin-sentry reviewed Dec 16, 2024

View reviewed changes

proto/sentry_protos/snuba/v1/endpoint_get_traces.proto Show resolved Hide resolved

phacops requested a review from colin-sentry December 16, 2024 20:02

davidtsuk reviewed Dec 16, 2024

View reviewed changes

proto/sentry_protos/snuba/v1/endpoint_get_traces.proto Outdated Show resolved Hide resolved

colin-sentry reviewed Dec 16, 2024

View reviewed changes

kylemumma approved these changes Dec 16, 2024

View reviewed changes

volokluev reviewed Dec 16, 2024

View reviewed changes

phacops requested a review from volokluev December 17, 2024 20:48

Specify which event type the condition targets

ce87464

phacops added 2 commits December 18, 2024 11:02

Add a root span duration field

6cb9087

Refactor and rename a few types

03d8e66

phacops requested review from colin-sentry and davidtsuk December 18, 2024 16:14

Add comments

aaf24d9

Zylphrex reviewed Dec 18, 2024

View reviewed changes

phacops added 2 commits December 18, 2024 14:24

Add a project ID attribute

a5100ec

Remove the possibility to ask for specific types for those hard-coded…

bbf4e65

… attributes

phacops requested a review from Zylphrex December 18, 2024 19:31

Zylphrex approved these changes Dec 18, 2024

View reviewed changes

davidtsuk approved these changes Dec 18, 2024

View reviewed changes

Add type back in TraceAttribute

2ddf2fa

phacops merged commit ff28c24 into main Dec 23, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eap): Add a GetTraces endpoint #71

feat(eap): Add a GetTraces endpoint #71

phacops commented Dec 13, 2024 •

edited

Loading

colin-sentry Dec 16, 2024 •

edited

Loading

phacops Dec 18, 2024

kylemumma left a comment

volokluev commented Dec 16, 2024

volokluev Dec 16, 2024

phacops Dec 17, 2024 •

edited

Loading

volokluev Dec 17, 2024

phacops Dec 17, 2024 •

edited

Loading

volokluev Dec 17, 2024

volokluev Dec 17, 2024

phacops Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

Zylphrex Dec 18, 2024

phacops Dec 18, 2024

Zylphrex Dec 18, 2024

phacops Dec 18, 2024

Zylphrex Dec 18, 2024

		// List of attributes we want to query.
		repeated TraceAttribute attributes = 6;

feat(eap): Add a GetTraces endpoint #71

feat(eap): Add a GetTraces endpoint #71

Conversation

phacops commented Dec 13, 2024 • edited Loading

colin-sentry Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylemumma left a comment

Choose a reason for hiding this comment

volokluev commented Dec 16, 2024

Choose a reason for hiding this comment

phacops Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phacops Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phacops Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phacops commented Dec 13, 2024 •

edited

Loading

colin-sentry Dec 16, 2024 •

edited

Loading

phacops Dec 17, 2024 •

edited

Loading

phacops Dec 17, 2024 •

edited

Loading

phacops Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading