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

add start and end parameters #4352

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

alburthoffman
Copy link

@alburthoffman alburthoffman commented Mar 29, 2023

Which problem is this PR solving?

Resolves 4150

Short description of the changes

allow optional time parameters when querying trace by id

@alburthoffman alburthoffman requested a review from a team as a code owner March 29, 2023 17:53
@alburthoffman
Copy link
Author

@pavolloffay @yurishkuro could u help with this PR?

Signed-off-by: alburthoffman <alburthoffman@gmail.com>
alburthoffman and others added 4 commits May 25, 2023 11:50
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

I updated IDL & regenerated typed

@@ -61,6 +61,13 @@ type Reader interface {
FindTraceIDs(ctx context.Context, query *TraceQueryParameters) ([]model.TraceID, error)
}

// TraceIDQueryParameters contains parameters of a trace id query.
type TraceIDQueryParameters struct {
Copy link
Member

Choose a reason for hiding this comment

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

this is rather unwieldy name, can we shorted to TraceIDQuery ?

@@ -36,7 +36,7 @@ type Reader interface {
// GetTrace retrieves the trace with a given id.
//
// If no spans are stored for this trace, it returns ErrTraceNotFound.
GetTrace(ctx context.Context, traceID model.TraceID) (*model.Trace, error)
GetTrace(ctx context.Context, query *TraceIDQueryParameters) (*model.Trace, error)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GetTrace(ctx context.Context, query *TraceIDQueryParameters) (*model.Trace, error)
GetTrace(ctx context.Context, query TraceIDQueryParameters) (*model.Trace, error)

The query is required, no reason to pass via pointer

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

I updated IDL & regenerated typed

@yurishkuro
Copy link
Member

The Proto file changes that include time stamps have been merged separately. This needs to be rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: time parameters for querying trace by id
2 participants