Instrument instances types #1484

rubenvp8510 · 2021-06-24T05:12:51Z

Signed-off-by: Ruben Vargas [email protected]

Which problem is this PR solving?

First approach of instrumenting Jaeger operator to know more about operands.
This report the number of instances and use labels for stratgegy, storage and agentMode

Short description of the changes

Use Prometheus exporter and OpenTelemety metrics API, created a valueObserver and query for instances manages by the operator in every callback. The reason of using a valueObserver is because I cannot relies on reconciliation process and using UpDownCounter instrument because the reconciliation process can be execute multiple times for creation/update/delete and I can't distinguish when is an update or creation.

pavolloffay · 2021-06-24T12:21:03Z

pkg/metrics/bootstrap.go

+// Bootstrap prepares a new tracer to be used by the operator
+func Bootstrap(ctx context.Context, namespace string, client client.Client) {
+	tracer := otel.GetTracerProvider().Tracer(v1.CustomMetricsTracer)
+	ctx, span := tracer.Start(ctx, "bootstrap")


is this span recording events/ reported?

If the bootstrap creates tracer than the span might be noop/default if it is created before initialization.

The tracer is already created when this bootstraping is called, this bootstraps the metrics for Open Telemetry,

pavolloffay · 2021-06-24T12:24:18Z

pkg/metrics/bootstrap.go

+const meterName = "jaegertracing.io/jaeger"
+
+// Bootstrap prepares a new tracer to be used by the operator
+func Bootstrap(ctx context.Context, namespace string, client client.Client) {


should this return some kind of closer interface to flush the spans on termination event?

When is this method called? Periodically or just once?

This is called just once.

pavolloffay · 2021-06-24T12:28:12Z

pkg/metrics/instances.go

+func (i *instancesMetric) strategy(jaeger v1.Jaeger) string {
+	strategy := string(jaeger.Spec.Strategy)
+	if strategy == "" {
+		return "allinone"


if this is the convention if strategy is empty then we should define a method on the Spec object to return a strategy and set the allinone as default.

The same for memory

and agent sidecar

They are set via Defaulters, but might be empty when running locally (make run) and/or in tests.

are we using Defaulters in this version of the operator? I started to do it on v2 but IIRC we don't use it here.

True. We have a normalizer in this operator.

pkg/apis/jaegertracing/v1/const.go

pkg/metrics/bootstrap.go

pkg/metrics/instances.go

jpkrohling · 2021-06-24T12:51:34Z

pkg/metrics/instances.go

+func (i *instancesMetric) strategy(jaeger v1.Jaeger) string {
+	strategy := string(jaeger.Spec.Strategy)
+	if strategy == "" {
+		return "allinone"


They are set via Defaulters, but might be empty when running locally (make run) and/or in tests.

pkg/metrics/bootstrap.go

pkg/metrics/instances.go

jpkrohling · 2021-06-24T13:03:19Z

pkg/metrics/instances.go

+	ctx, span := tracer.Start(ctx, "setup-jaeger-instances")
+	defer span.End()
+	meter := global.Meter(meterName)
+	_, err := meter.NewInt64ValueObserver("operator_jaeger_instances", i.callback,


With which frequency is the callback called?

IIRC this is called per collection interval.

codecov · 2021-06-24T16:13:10Z

Codecov Report

Merging #1484 (36877a0) into master (b27ec96) will decrease coverage by 0.37%.
The diff coverage is 67.92%.

@@            Coverage Diff             @@
##           master    #1484      +/-   ##
==========================================
- Coverage   88.28%   87.91%   -0.38%     
==========================================
  Files          90       92       +2     
  Lines        5660     5766     +106     
==========================================
+ Hits         4997     5069      +72     
- Misses        503      532      +29     
- Partials      160      165       +5

Impacted Files	Coverage Δ
pkg/metrics/bootstrap.go	`0.00% <0.00%> (ø)`
pkg/metrics/instances.go	`87.80% <87.80%> (ø)`

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 b27ec96...36877a0. Read the comment docs.

rubenvp8510 · 2021-06-26T03:06:29Z

Address all comments, added an screen-shot with the visualization of the metrics.

pavolloffay · 2021-06-28T09:25:15Z

Address all comments, added an screen-shot with the visualization of the metrics.

If the metrics are exposed in p8s nedpoint. It might be better to paste the metrics here in the text form. At least for me it is the best way to understand what is being exposed.

rubenvp8510 · 2021-06-29T04:42:03Z

Address all comments, added an screen-shot with the visualization of the metrics.

If the metrics are exposed in p8s nedpoint. It might be better to paste the metrics here in the text form. At least for me it is the best way to understand what is being exposed.

# HELP operator_jaeger_instances Number of jaeger instances in cluster
# TYPE operator_jaeger_instances gauge
operator_jaeger_instances{agent="sidecar",service_instance_id="default.jaeger-operator",service_name="jaeger-operator",service_namespace="default",service_version="v1.23.0.redhat1-7-g97c68db8",storage="elasticsearch",strategy="production"} 1
operator_jaeger_instances{agent="sidecar",service_instance_id="default.jaeger-operator",service_name="jaeger-operator",service_namespace="default",service_version="v1.23.0.redhat1-7-g97c68db8",storage="memory",strategy="allinone"} 1

pavolloffay · 2021-06-29T09:16:13Z

The metrics look good. I would rename agent to agent_strategy

Are the following labels needed? What do they provide us?

service_instance_id="default.jaeger-operator",service_name="jaeger-operator",service_namespace="default

rubenvp8510 · 2021-06-29T14:30:29Z

The metrics look good. I would rename agent to agent_strategy

Are the following labels needed? What do they provide us?
service_instance_id="default.jaeger-operator",service_name="jaeger-operator",service_namespace="default

I don't think we need it, we can get rid of it

WDYT? @jpkrohling

jpkrohling · 2021-06-30T09:27:13Z

If we keep the name/namespace labels, we will have high cardinality here. In fact, I would even split the metrics into this:

jaeger_operator_instances_agent_modes{mode=sidecar} 1
jaeger_operator_instances_agent_modes{mode=daemonset} 1
jaeger_operator_instances_storage_types{type=memory} 1
jaeger_operator_instances_storage_types{type=elasticsearch} 1
jaeger_operator_instances_strategies{type=production} 1
jaeger_operator_instances_strategies{type=allinone} 1
jaeger_operator_instances_versions{version=1.23.0} 1
jaeger_operator_instances_versions{version=1.24.0} 1

I'm not familiar with the OpenTelemetry Metrics API, but OpenCensus had a separation between counters and views: you'd have only one counter here, reporting how the instance looks like (agent mode, storage, strategy, version), and four different views, one for each metric I listed above.

With that, we only have a limited number of time series, the last one being the only one that can potentially grow "indefinitely".

Note that my proposal also changes the prefix, to be inline with the other Jaeger components ("jaeger_collector_...", "jaeger_agent_...")

rubenvp8510 · 2021-07-04T17:53:09Z

I changed the metrics using the suggested way:

# HELP jaeger_operator_instances_agent_strategies Number of instances per agent strategy
# TYPE jaeger_operator_instances_agent_strategies gauge
jaeger_operator_instances_agent_strategies{type="sidecar"} 1
# HELP jaeger_operator_instances_storage_types Number of instances per storage type
# TYPE jaeger_operator_instances_storage_types gauge
jaeger_operator_instances_storage_types{type="memory"} 1
# HELP jaeger_operator_instances_strategies Number of instances per strategy type
# TYPE jaeger_operator_instances_strategies gauge
jaeger_operator_instances_strategies{type="allinone"} 1

pkg/cmd/start/bootstrap.go

pkg/metrics/instances.go

pkg/apis/jaegertracing/v1/const.go

pkg/metrics/instances.go

Signed-off-by: Ruben Vargas <[email protected]>

Co-authored-by: Juraci Paixão Kröhling <[email protected]> Signed-off-by: Ruben Vargas <[email protected]>

- Fixed import orders - Better error logging and handling - Some code linting fixes Signed-off-by: Ruben Vargas <[email protected]>

…ce attributes Signed-off-by: Ruben Vargas <[email protected]>

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 · 2021-07-10T05:22:19Z

@jpkrohling @pavolloffay Please give it another review, I added tests and a flag to enable/disable this feature.

Signed-off-by: Ruben Vargas <[email protected]>

pkg/metrics/bootstrap.go

pkg/cmd/start/main.go

pkg/metrics/bootstrap.go

Signed-off-by: Ruben Vargas <[email protected]>

…r into instrument-op

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 · 2021-07-12T18:15:05Z

I address all comments, removed the flag, I think this is ready for another review and hopefully a merge =)

jpkrohling · 2021-07-13T07:52:24Z

pkg/metrics/bootstrap.go

+	c := controller.New(
+		processor.New(
+			selector.NewWithHistogramDistribution(
+				histogram.WithExplicitBoundaries(config.DefaultHistogramBoundaries),


What are the boundaries defined here? Do they make sense for us?

None of that is applicable to us. for now. That is why I just set default values.

What are the default values? If we don't have histograms, why do we configure them? Can't we just leave this out?

What are the default values? If we don't have histograms, why do we configure them? Can't we just leave this out?

I don't know, we are not using it. I can't I need to pass and aggregation selector to the New method. :/

// NewWithHistogramDistribution returns a simple aggregator selector // that uses histogram aggregators for `ValueRecorder` instruments. // This selector is a good default choice for most metric exporters.

From this comment I'm assuming this is the best "default" configuration.

rubenvp8510 force-pushed the instrument-op branch 2 times, most recently from fc24db9 to 36fdb62 Compare June 24, 2021 05:34

pavolloffay reviewed Jun 24, 2021

View reviewed changes

jpkrohling reviewed Jun 24, 2021

View reviewed changes

rubenvp8510 force-pushed the instrument-op branch 2 times, most recently from 083af9a to a5c078a Compare June 25, 2021 05:33

rubenvp8510 requested review from jpkrohling and pavolloffay June 26, 2021 03:06

rubenvp8510 force-pushed the instrument-op branch from b4de1eb to 6a5cec6 Compare June 29, 2021 05:45

rubenvp8510 force-pushed the instrument-op branch from b69969e to 8000b2c Compare July 4, 2021 22:46

jpkrohling reviewed Jul 5, 2021

View reviewed changes

pkg/cmd/start/bootstrap.go Outdated Show resolved Hide resolved

pkg/metrics/instances.go Show resolved Hide resolved

pkg/apis/jaegertracing/v1/const.go Outdated Show resolved Hide resolved

pkg/metrics/instances.go Show resolved Hide resolved

rubenvp8510 force-pushed the instrument-op branch 2 times, most recently from 68c4a90 to eec4c90 Compare July 10, 2021 05:18

rubenvp8510 and others added 9 commits July 10, 2021 00:20

Instrument instances types

827fd24

Signed-off-by: Ruben Vargas <[email protected]>

Update pkg/apis/jaegertracing/v1/const.go

a7d9ca0

Co-authored-by: Juraci Paixão Kröhling <[email protected]> Signed-off-by: Ruben Vargas <[email protected]>

Address following comments:

86ee2fe

- Fixed import orders - Better error logging and handling - Some code linting fixes Signed-off-by: Ruben Vargas <[email protected]>

Create resource for metrics with the servicename, version and namespa…

05d29f1

…ce attributes Signed-off-by: Ruben Vargas <[email protected]>

Separate instance metrics by agent, storage and strategy

8277fba

Signed-off-by: Ruben Vargas <[email protected]>

Change label key name

e6fa54e

Signed-off-by: Ruben Vargas <[email protected]>

Rename metrics

1eb48a1

Signed-off-by: Ruben Vargas <[email protected]>

Lowercase metrics constants

0b811c9

Signed-off-by: Ruben Vargas <[email protected]>

Minor style issues fixed

38e5a81

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 added 3 commits July 10, 2021 00:20

Metrics instrumentation tests

0a2a1ed

Signed-off-by: Ruben Vargas <[email protected]>

Add tests to metrics

3330035

Signed-off-by: Ruben Vargas <[email protected]>

Add flags to enable/disable operand metrics

f96b438

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the instrument-op branch from eec4c90 to f96b438 Compare July 10, 2021 05:21

rubenvp8510 requested a review from jpkrohling July 10, 2021 05:21

rubenvp8510 and others added 2 commits July 10, 2021 00:37

Minor fixes

7106677

Signed-off-by: Ruben Vargas <[email protected]>

Merge branch 'master' into instrument-op

91221bf

jpkrohling reviewed Jul 12, 2021

View reviewed changes

pkg/metrics/bootstrap.go Outdated Show resolved Hide resolved

pkg/cmd/start/main.go Outdated Show resolved Hide resolved

pkg/metrics/bootstrap.go Outdated Show resolved Hide resolved

pkg/metrics/bootstrap.go Outdated Show resolved Hide resolved

rubenvp8510 added 3 commits July 12, 2021 08:49

Import orders, code style comments addresed

15efd7c

Signed-off-by: Ruben Vargas <[email protected]>

Merge branch 'instrument-op' of github.com:rubenvp8510/jaeger-operato…

d65b2e0

…r into instrument-op

Remove metrics enabled flag

725d74c

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 mentioned this pull request Jul 13, 2021

Remove tracing-enabled flag #1506

Open

jpkrohling reviewed Jul 13, 2021

View reviewed changes

Merge branch 'master' into instrument-op

36877a0

jpkrohling approved these changes Jul 13, 2021

View reviewed changes

jpkrohling enabled auto-merge (squash) July 13, 2021 15:42

jpkrohling merged commit e6b4930 into jaegertracing:master Jul 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrument instances types #1484

Instrument instances types #1484

rubenvp8510 commented Jun 24, 2021 •

edited

Loading

pavolloffay Jun 24, 2021

rubenvp8510 Jun 26, 2021

pavolloffay Jun 24, 2021

pavolloffay Jun 24, 2021

rubenvp8510 Jun 24, 2021

pavolloffay Jun 24, 2021

pavolloffay Jun 24, 2021

pavolloffay Jun 24, 2021

jpkrohling Jun 24, 2021

rubenvp8510 Jun 24, 2021

jpkrohling Jun 25, 2021

jpkrohling Jun 24, 2021

jpkrohling Jun 24, 2021

rubenvp8510 Jun 25, 2021

codecov bot commented Jun 24, 2021 •

edited

Loading

rubenvp8510 commented Jun 26, 2021

pavolloffay commented Jun 28, 2021

rubenvp8510 commented Jun 29, 2021 •

edited

Loading

pavolloffay commented Jun 29, 2021

rubenvp8510 commented Jun 29, 2021

jpkrohling commented Jun 30, 2021 •

edited

Loading

rubenvp8510 commented Jul 4, 2021 •

edited

Loading

rubenvp8510 commented Jul 10, 2021 •

edited

Loading

rubenvp8510 commented Jul 12, 2021

jpkrohling Jul 13, 2021

rubenvp8510 Jul 13, 2021

jpkrohling Jul 13, 2021

rubenvp8510 Jul 13, 2021 •

edited

Loading

rubenvp8510 Jul 13, 2021

Instrument instances types #1484

Instrument instances types #1484

Conversation

rubenvp8510 commented Jun 24, 2021 • edited Loading

Which problem is this PR solving?

Short description of the changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 24, 2021 • edited Loading

Codecov Report

rubenvp8510 commented Jun 26, 2021

pavolloffay commented Jun 28, 2021

rubenvp8510 commented Jun 29, 2021 • edited Loading

pavolloffay commented Jun 29, 2021

rubenvp8510 commented Jun 29, 2021

jpkrohling commented Jun 30, 2021 • edited Loading

rubenvp8510 commented Jul 4, 2021 • edited Loading

rubenvp8510 commented Jul 10, 2021 • edited Loading

rubenvp8510 commented Jul 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenvp8510 commented Jun 24, 2021 •

edited

Loading

codecov bot commented Jun 24, 2021 •

edited

Loading

rubenvp8510 commented Jun 29, 2021 •

edited

Loading

jpkrohling commented Jun 30, 2021 •

edited

Loading

rubenvp8510 commented Jul 4, 2021 •

edited

Loading

rubenvp8510 commented Jul 10, 2021 •

edited

Loading

rubenvp8510 Jul 13, 2021 •

edited

Loading