Add optional Opentracing #111
Conversation
ghost
requested a review
from 
|
Close |
util/tele/tele.go
Outdated
| // Return tracer | ||
| func Tracer() trace.Tracer { | ||
| return tracer{ | ||
| Tracer: otel.Tracer("capz"), |
cloud/opentracing/traces.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| fmt.Printf("Begin registering opentelemetry\n") |
| @@ -7,6 +7,7 @@ | |||
| - [Troubleshooting](./topics/troubleshooting.md) | |||
| - [Cluster-Autoscaler](./topics/cluster-autoscaler.md) | |||
| - [How to config](./topics/config.md) | |||
| - [How to enable tracing](./topics/tracing.md) | |||
There was a problem hiding this comment.
Good job.
I'm still thinking that this feature is too early to integrate. Right now, we do not use this tool to diagnostic performance.
Could we stall this PR and wait when we need to optimize ? (CC @jerome-jutteau)
docs/src/topics/tracing.md
Outdated
| @@ -0,0 +1,10 @@ | |||
| # Add tracing | |||
|
|
|||
| You can use jaeger or Zipkin as backend for opentelemetry. | |||
There was a problem hiding this comment.
- "Jaeger"
- "OpenTelemetry"
controllers/osccluster_controller.go
Outdated
| ctx, _, publicIpDone := tele.StartSpanWithLogger(ctx, "controllers.OscClusterControllers.publicIpReconcile") | ||
|
|
||
| publicIpSvc := r.getPublicIpSvc(ctx, *clusterScope) | ||
| reconcilePublicIp, err := reconcilePublicIp(ctx, clusterScope, publicIpSvc) | ||
| if err != nil { | ||
| clusterScope.Error(err, "failed to reconcile publicIp") | ||
| conditions.MarkFalse(osccluster, infrastructurev1beta1.PublicIpsReadyCondition, infrastructurev1beta1.PublicIpsFailedReason, clusterv1.ConditionSeverityWarning, err.Error()) | ||
| publicIpDone() | ||
| return reconcilePublicIp, err | ||
| } | ||
| publicIpDone() | ||
| conditions.MarkTrue(osccluster, infrastructurev1beta1.PublicIpsReadyCondition) |
There was a problem hiding this comment.
Taking a step back mae me realize that the suitable place to these functions would be directly in each function (for example in this case at the beginning of the reconcilePublicIp)
There was a problem hiding this comment.
Maybye keep here so that it is simple to add new ones in the futur ?
What do you think ?
There was a problem hiding this comment.
@outscale-mdr Are you ok with the changes ?
There was a problem hiding this comment.
It is easier to put the code for tracing at the beginning of the function rather than in the middle of another
|
To be freeze |
ghost
closed this
What type of PR is this?
/kind feature
What this PR does / why we need it:
Add optional opentracing with:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #130
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
TODOs: