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 metrics reader interface and gen proto #2977

Merged

Conversation

albertteoh
Copy link
Contributor

Signed-off-by: albertteoh [email protected]

Which problem is this PR solving?

Short description of the changes

  • Adds the Metrics Reader interface bridging the gRPC and HTTP API handlers with the underlying metrics storage implementation.
  • Adds the generated proto API and data model code used by the reader interface for returning the proto modelled metrics data as a response.

@albertteoh albertteoh requested a review from a team as a code owner May 4, 2021 12:10
@codecov
Copy link

codecov bot commented May 4, 2021

Codecov Report

Merging #2977 (30fcfec) into master (f076065) will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2977      +/-   ##
==========================================
+ Coverage   95.90%   95.96%   +0.06%     
==========================================
  Files         223      223              
  Lines        9720     9720              
==========================================
+ Hits         9322     9328       +6     
+ Misses        328      324       -4     
+ Partials       70       68       -2     
Impacted Files Coverage Δ
plugin/storage/integration/integration.go 77.90% <0.00%> (+0.55%) ⬆️
cmd/query/app/static_handler.go 96.77% <0.00%> (+4.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f076065...30fcfec. Read the comment docs.

@@ -0,0 +1,2062 @@
// Code generated by protoc-gen-gogo. DO NOT EDIT.
Copy link
Member

Choose a reason for hiding this comment

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

service.pb.go is an odd name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I'd originally tried query.proto but protoc complains about filenames shadowing one another with the existing query.proto, which is why I chose service.proto but agree it could be better named.

I've gone with metricsquery.proto. Let me know if you prefer another name.

"context"
"time"

"github.com/jaegertracing/jaeger/proto-gen/api_v2"
Copy link
Member

Choose a reason for hiding this comment

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

perhaps we should move the gen-types under api_v2/metrics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1

@yurishkuro yurishkuro merged commit e08d812 into jaegertracing:master May 6, 2021
@jpkrohling jpkrohling added this to the Release 1.23.0 milestone Jun 4, 2021
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.

3 participants