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

Implemented a second service for the collector #339

Merged

Conversation

jpkrohling
Copy link
Contributor

Fixes #264

Signed-off-by: Juraci Paixão Kröhling [email protected]

@jpkrohling
Copy link
Contributor Author

This change is Reviewable

@jpkrohling
Copy link
Contributor Author

E2E tests are passing:

Running Smoke end-to-end tests...
ok  	github.com/jaegertracing/jaeger-operator/test/e2e	166.810s
Creating namespace default
Running Cassandra end-to-end tests...
ok  	github.com/jaegertracing/jaeger-operator/test/e2e	116.122s
Running Elasticsearch end-to-end tests...
ok  	github.com/jaegertracing/jaeger-operator/test/e2e	106.595s

@codecov
Copy link

codecov bot commented Mar 20, 2019

Codecov Report

Merging #339 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #339      +/-   ##
=========================================
+ Coverage   89.75%   89.8%   +0.04%     
=========================================
  Files          64      64              
  Lines        2997    3011      +14     
=========================================
+ Hits         2690    2704      +14     
  Misses        207     207              
  Partials      100     100
Impacted Files Coverage Δ
pkg/inventory/service.go 87.5% <100%> (+0.83%) ⬆️
pkg/service/collector.go 100% <100%> (ø) ⬆️
pkg/inject/sidecar.go 100% <100%> (ø) ⬆️
pkg/deployment/all-in-one.go 100% <100%> (ø) ⬆️
pkg/deployment/collector.go 100% <100%> (ø) ⬆️
pkg/service/query.go 100% <100%> (ø) ⬆️
pkg/deployment/agent.go 100% <100%> (ø) ⬆️

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 0772935...cec8456. Read the comment docs.


// GetNameForHeadlessCollectorService returns the headless service name for the collector in this Jaeger instance
func GetNameForHeadlessCollectorService(jaeger *v1.Jaeger) string {
return fmt.Sprintf("%s-collector-headless", jaeger.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This could take the output from GetNameForCollectorService and append -headless. Only minor though feel free to ignore.

@jpkrohling
Copy link
Contributor Author

I manually tested the load balancing for the communication between the agent => collector and it's still working after this PR.

https://gist.github.com/jpkrohling/0de49a9c23c24f4db07427aee7206a0e

When setting the JAEGER_ENDPOINT, pointing to the cluster IP address, each tracer instance sends spans to the same collector, which can probably be explained by OkHttp caching the connection.

Here's the YAML definition for the test application:

apiVersion: apps/v1
kind: Deployment
metadata:
  name: myapp
spec:
  selector:
    matchLabels:
      app: myapp
  template:
    metadata:
      labels:
        app: myapp
    spec:
      containers:
      - name: myapp
        image: jaegertracing/vertx-create-span:operator-e2e-tests
        env:
        - name: JAEGER_ENDPOINT
          value: "http://simple-prod-collector:14268/api/traces"

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
@jpkrohling jpkrohling force-pushed the 264-Dual-services-for-collector branch from bc3d55f to cec8456 Compare March 20, 2019 16:57
@jpkrohling
Copy link
Contributor Author

@adinunzio84, would you take a look at this PR and see if it solves the problem you reported?

@objectiser
Copy link
Contributor

@jpkrohling Could you scale up the test app, and see whether the use of JAEGER_ENDPOINT also load balances?

@jpkrohling
Copy link
Contributor Author

It does. Doesn't look as even as when using the agent, but it does balance. Note that the output here is skewed, as it still has data from the first run:

$ curl localhost:14268/metrics 2>/dev/null | grep jaeger_collector_spans_received_total 
jaeger_collector_spans_received_total{debug="false",format="jaeger",svc="inventory"} 800
jaeger_collector_spans_received_total{debug="false",format="jaeger",svc="order"} 500

$ curl localhost:14268/metrics 2>/dev/null | grep jaeger_collector_spans_received_total 
jaeger_collector_spans_received_total{debug="false",format="jaeger",svc="inventory"} 2000
jaeger_collector_spans_received_total{debug="false",format="jaeger",svc="order"} 1000

$ curl localhost:14268/metrics 2>/dev/null | grep jaeger_collector_spans_received_total 
jaeger_collector_spans_received_total{debug="false",format="jaeger",svc="inventory"} 400
jaeger_collector_spans_received_total{debug="false",format="jaeger",svc="order"} 2500

@objectiser
Copy link
Contributor

@jpkrohling ok great!

@jpkrohling
Copy link
Contributor Author

I'm merging this, but let me know if there's anything missing, @adinunzio84.

@jpkrohling jpkrohling merged commit 3ff2274 into jaegertracing:master Mar 21, 2019
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.

2 participants