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.

Sign up for GitHub

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

Jump to bottom

feat: implement gRPC storage plugin for metrics #4414

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

feat: implement gRPC storage plugin for metrics #4414

wants to merge 2 commits into from

Conversation

jacobmarble
Copy link
Contributor

@jacobmarble jacobmarble commented Apr 26, 2023 •
edited

Which problem is this PR solving?

Resolves #4413

Short description of the changes

  • define the metrics gRPC API in protos
  • refactor: move two subpackages of plugin/storage/grpc to pkg/grpc
  • implement the gRPC metrics plugin

A POC is implemented here: influxdata/influxdb-observability#239

TODO

  • initial review by maintainers
  • add unit tests (new _test.go files with TODO comments)

@codecov
Copy link

codecov bot commented Apr 26, 2023 •
edited

Codecov Report

Patch coverage: 32.12% and project coverage change: -0.80 ⚠️

Comparison is base (3021d31) 97.06% compared to head (61b0f86) 96.26%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4414      +/-   ##
==========================================
- Coverage   97.06%   96.26%   -0.80%     
==========================================
  Files         300      303       +3     
  Lines       17737    17955     +218     
==========================================
+ Hits        17217    17285      +68     
- Misses        417      558     +141     
- Partials      103      112       +9
Flag Coverage Δ
unittests 96.26% <32.12%> (-0.80%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
plugin/metrics/factory.go 88.63% <0.00%> (-11.37%) ⬇️
plugin/metrics/grpc/factory.go 0.00% <0.00%> (ø)
plugin/metrics/grpc/options.go 0.00% <0.00%> (ø)
plugin/storage/grpc/grpc.go 0.00% <0.00%> (ø)
plugin/storage/grpc/shared/metrics.go 0.00% <0.00%> (ø)
plugin/storage/grpc/shared/plugin.go 0.00% <0.00%> (ø)
plugin/storage/grpc/shared/grpc_client.go 84.91% <50.00%> (-0.80%) ⬇️
plugin/storage/grpc/shared/grpc_handler.go 74.11% <73.40%> (-0.36%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jacobmarble jacobmarble force-pushed the jgm-plugin-metrics-grpc branch 7 times, most recently from 0c5d701 to 0707fd9 Compare May 2, 2023 16:56
@jacobmarble jacobmarble force-pushed the jgm-plugin-metrics-grpc branch 4 times, most recently from 4872990 to 619a295 Compare May 3, 2023 21:39
@jacobmarble jacobmarble mentioned this pull request May 3, 2023
Draft
@jacobmarble jacobmarble marked this pull request as ready for review May 3, 2023 21:51
@jacobmarble jacobmarble requested a review from a team as a code owner May 3, 2023 21:51
@jacobmarble jacobmarble force-pushed the jgm-plugin-metrics-grpc branch 2 times, most recently from ae20758 to 96b3cac Compare May 8, 2023 20:32
@jacobmarble jacobmarble mentioned this pull request May 10, 2023
Open
yurishkuro
yurishkuro reviewed May 16, 2023
View reviewed changes
pkg/grpc/grpc.go Outdated Show resolved Hide resolved
jacobmarble and others added 2 commits May 22, 2023 20:55
@jacobmarble
feat: Add protos for metrics gRPC plugin
305cf09 
Signed-off-by: Jacob Marble <jacobmarble@gmail.com>
@jacobmarble
feat: Implement gRPC metrics plugin
61b0f86 
Signed-off-by: Jacob Marble <jacobmarble@gmail.com>
yurishkuro
yurishkuro reviewed May 27, 2023
View reviewed changes
Copy link
Member

@yurishkuro yurishkuro left a comment •
edited

Choose a reason for hiding this comment

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

It's a bit hard to grok from the changes, but what does this mean in practice that a MetricsReader service is added to the main storage plugin? Since it's controlled by capabilities API, it means existing storages don't have to implement it, which is good. But I am also interested in the reverse - what if I am using one of standard backends for span storage and want grpc-remote implementation for metrics store? Is that possible?

plugin/metrics/factory.go
@@ -33,6 +34,7 @@ const (
disabledStorageType = ""

prometheusStorageType = "prometheus"
grpcStorageType = "grpc-plugin"
Copy link
Member

Choose a reason for hiding this comment

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

So we have --metrics-storage-type=grpc-plugin but then factory flags under --grpc-metrics.*? I can't think if a better name, but at least we should keep these in sync, so let's go with grpc-metrics

plugin/metrics/factory.go
@@ -67,6 +69,8 @@ func (f *Factory) getFactoryOfType(factoryType string) (storage.MetricsFactory,
switch factoryType {
case prometheusStorageType:
return prometheus.NewFactory(), nil
case grpcStorageType:
return grpc.NewFactory(), nil
Copy link
Member

Choose a reason for hiding this comment

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

could we add a test?

plugin/metrics/grpc/options_test.go

package grpc

// TODO write tests
Copy link
Member

Choose a reason for hiding this comment

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

add tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro left review comments

@joe-elliott joe-elliott Awaiting requested review from joe-elliott joe-elliott is a code owner automatically assigned from jaegertracing/jaeger-maintainers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature]: gRPC storage plugin for metrics
2 participants
@jacobmarble @yurishkuro