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 support for GRPC storage plugin #1517

Merged
merged 1 commit into from
Jul 26, 2021

Conversation

pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Jul 23, 2021

Resolves #1042
Supersedes #1045

How does it work:

User has to specify grpc-plugin as storage.type and image with the plugin binary in storage.grpcPlugin.image. The image has to copy plugin binary into /plugin/ directory. At the moment this is not configurable. If there will be requests we could make that configurable in the CR.

The image is used for init container. At jaeger startup the init container copies the plugin binary into volume. This makes the binary visible to Jaeger.

The flags for the storage plugin (bibary, configuration-file, log-level) are provided in storage.options, to be consistent with other storage implementations. The configuration file for the storage plugin has to be explicitly configured in storage volumes as a config map (the user is responsible for creating it).

apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: clickhouse-grpc-plugin
spec:
  storage:
    type: grpc-plugin
    grpcPlugin:
      image: ghcr.io/pavolloffay/jaeger-clickhouse:0.4.1
    options:
      grpc-storage-plugin:
        binary: /plugin/jaeger-clickhouse
        configuration-file: /plugin-config/config.yaml
        log-level: debug
  volumeMounts:
    - name: plugin-config
      mountPath: /plugin-config
  volumes:
    - name: plugin-config
      configMap:
        name: jaeger-clickhouse

This PR does not contain e2e tests. The e2e tests will be added in a separate PR.

@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #1517 (9020585) into master (25e9291) will decrease coverage by 0.00%.
The diff coverage is 87.87%.

❗ Current head 9020585 differs from pull request most recent head 5c5e848. Consider uploading reports for the commit 5c5e848 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1517      +/-   ##
==========================================
- Coverage   87.93%   87.93%   -0.01%     
==========================================
  Files          92       93       +1     
  Lines        5778     5811      +33     
==========================================
+ Hits         5081     5110      +29     
- Misses        532      534       +2     
- Partials      165      167       +2     
Impacted Files Coverage Δ
pkg/metrics/bootstrap.go 0.00% <ø> (ø)
pkg/apis/jaegertracing/v1/jaeger_types.go 87.50% <33.33%> (-12.50%) ⬇️
pkg/storage/grpc_plugin.go 90.90% <90.90%> (ø)
pkg/deployment/all_in_one.go 100.00% <100.00%> (ø)
pkg/deployment/collector.go 100.00% <100.00%> (ø)
pkg/deployment/ingester.go 100.00% <100.00%> (ø)
pkg/deployment/query.go 100.00% <100.00%> (ø)

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 25e9291...5c5e848. Read the comment docs.

rubenvp8510
rubenvp8510 previously approved these changes Jul 24, 2021
@rubenvp8510
Copy link
Collaborator

I think we should add documentation on how this works. (something similar to the description of this PR)

@@ -5,6 +5,8 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/require"
Copy link
Collaborator

@rubenvp8510 rubenvp8510 Jul 24, 2021

Choose a reason for hiding this comment

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

I think the import order here is not correct, please verify

Copy link
Member Author

Choose a reason for hiding this comment

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

it passes CI check. We should perhaps use different tooling to catch this on CI

@pavolloffay
Copy link
Member Author

re docs- yes definitely I will add them to docs repo.

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@pavolloffay pavolloffay merged commit 4823277 into jaegertracing:master Jul 26, 2021
johanneswuerbach added a commit to johanneswuerbach/jaeger-dynamodb that referenced this pull request Oct 6, 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.

Operator for storage plugin
2 participants