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

[extension/jaegerremotesampling] remote sampling gRPC support #6694 #12788

Conversation

frzifus
Copy link
Member

@frzifus frzifus commented Jul 30, 2022

Description:

This pr adds gRPC support to the jaegerremotesampling extension.

Link to tracking Issue:

Testing:

  • Unittest

Documentation:

@jpkrohling
Copy link
Member

Once this is ready to be reviewed, move it out of Draft status and/or ping me.

@frzifus frzifus marked this pull request as ready for review August 1, 2022 15:38
@frzifus frzifus requested a review from a team August 1, 2022 15:38
@frzifus frzifus requested a review from jpkrohling as a code owner August 1, 2022 15:38
@frzifus
Copy link
Member Author

frzifus commented Aug 1, 2022

@jpkrohling thanks :) Should be ready to review 👍

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

This looks really good. Before doing my manual tests on this, I wanted to confirm whether you performed yours and how you did it. Thank you for this PR!

extension/jaegerremotesampling/extension.go Outdated Show resolved Hide resolved
extension/jaegerremotesampling/extension.go Outdated Show resolved Hide resolved
extension/jaegerremotesampling/extension.go Outdated Show resolved Hide resolved
extension/jaegerremotesampling/extension.go Outdated Show resolved Hide resolved
extension/jaegerremotesampling/internal/grpc.go Outdated Show resolved Hide resolved
extension/jaegerremotesampling/internal/grpc.go Outdated Show resolved Hide resolved
@frzifus
Copy link
Member Author

frzifus commented Aug 1, 2022

Before doing my manual tests on this, I wanted to confirm whether you performed yours and how you did it.

I used the following configuration based on the test data located in the jaegerremotesampling folder and placed it into the project root (config.yaml).

receivers:
  otlp:
    protocols:
      grpc:
      http:

exporters:
  logging:
    logLevel: debug

extensions:
  pprof:
  jaegerremotesampling:
    source:
      reload_interval: 5s
      file: strategies.json

service:
  extensions: [jaegerremotesampling]
  pipelines:
    traces:
      receivers: [otlp]
      exporters: [logging]

The specified strategies.json is downloaded from the Jaeger project v1.36.0.
As a next step I registered the jaegerremotesampling factory in otelcontribcol(components), compiled the program and executed it.

$ make otelcontribcol && ./bin/otelcontribcol_linux_amd64 --config config.yaml

As counter part I used this small go program:

package main

import (
	"context"
	"fmt"
	"log"
	"time"

	jaegergrpc "github.com/jaegertracing/jaeger/cmd/agent/app/configmanager/grpc"
	"google.golang.org/grpc"
	"google.golang.org/grpc/credentials/insecure"
)

func main() {
	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
	defer cancel()
	grpcOptions := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithBlock()}
	conn, err := grpc.DialContext(ctx, "localhost:14250", grpcOptions...)
	if err != nil {
		log.Fatalf("failed to create gRPC connection to collector: %w", err)
	}
	defer conn.Close()

	mgr := jaegergrpc.NewConfigManager(conn)

	doReq := func(svc string) {
		ctx, cancel = context.WithTimeout(context.Background(), time.Minute)
		defer cancel()
		resp, err := mgr.GetSamplingStrategy(ctx, "svc")
		if err != nil {
			log.Fatal(err)
		}
		fmt.Printf("%+v\n", resp)
	}

	for {
		doReq("my-service")
		time.Sleep(15 * time.Second)
	}
}

Then I changed the param from 0.5 to 0.6 and back again.

@frzifus frzifus force-pushed the feat/jaeger_remote_sampling_grpc_support_#6694 branch from 7c940b1 to 4b29ce6 Compare August 1, 2022 21:54
@frzifus frzifus requested a review from jpkrohling August 1, 2022 21:54
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM

@jpkrohling
Copy link
Member

Looks like this is missing the changelog entry. Please, check the contributing guidelines, but basically, you'll need a file within the "unreleased" directory.

@frzifus
Copy link
Member Author

frzifus commented Aug 2, 2022

added 👍

@frzifus
Copy link
Member Author

frzifus commented Aug 4, 2022

@jpkrohling is there something else needed to continue? :)

@frzifus frzifus force-pushed the feat/jaeger_remote_sampling_grpc_support_#6694 branch from 1bda828 to 9db0bec Compare August 5, 2022 08:05
@frzifus
Copy link
Member Author

frzifus commented Aug 5, 2022

just rebased

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM

@jpkrohling jpkrohling merged commit 55cd915 into open-telemetry:main Aug 8, 2022
@frzifus frzifus deleted the feat/jaeger_remote_sampling_grpc_support_#6694 branch August 11, 2022 10:29
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.

Add the gRPC server
2 participants