Only do telemetry computation if telemetry is enabled in the node

[ValarDragon]

Summary

As detailed in other issues, the mutex locks and time.Now() syscalls' taken in telemetry are rather expensive for nodes. In general, nodes not using telemetry should not pay these costs.

Problem Definition

Nodes that aren't using telemetry should not pay the costs of telemetry.

Proposal

Somehow make the telemetry package know the result of its config option detailing whether or not its enabled. Then only do operations when its enabled. This is useful due to potential for mutex lock contention.

This doesn't solve the extraneous time.Now() call, but that should hopefully be solvable via other mechanisms. (e.g. not putting telemetry into hot loops / low level items) This can't be deadcode eliminated / constant folded, since telemetry enablement is a run-time flag, not a compile time flag

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[alexanderbez]

ACK, yeah this totally makes sense and TBH was an oversight on my part. I didn't realize how much time these calls took.

[ValarDragon]

To improve the time.Now() call, we can make that a telemetry function. So replace time.Now() in the defer statements with telemetry.StartTimer(). And the StartTimer function would be if telemetry enabled { return time.Now() } else { time.Time{} }

[ValarDragon]

Does anyone have any guidance on how we can get there to be a flag for whether or not telemetry is enabled? Some thoughts that come to mind for me:

• Does such a flag belong in the ctx
• Should it be a global variable in the telemetry package thats set on node startup?
• Should there be a telemetry object accessible via ctx?

I feel like putting a TelemetryEnabled flag in the context may be the simplest thing thats re-usable, but not at all sure its the right thing to do.

[alexanderbez]

@ValarDragon the SDK context type? I think that might be our only option. Alternatively, we have a global variable in the telemetry package.

[ValarDragon]

yeah meant SDK context type.

I feel like a global variable in telemetry will make the syntax for doing telemetry calls simpler at least, no idea if it would cause problems for integrators though. cc @jackzampolin

e.g. API with global variable

defer telemetry.ModuleMeasureSince(types.ModuleName, telemetry.StartTimer(), telemetry.MetricKeyBeginBlocker)

API choices with it in context:

if ctx.telemetryEnabled {
defer telemetry.ModuleMeasureSince(types.ModuleName, telemetry.StartTimer(), telemetry.MetricKeyBeginBlocker)`
}

or

defer telemetry.ModuleMeasureSince(ctx, types.ModuleName, telemetry.StartTimer(ctx), telemetry.MetricKeyBeginBlocker)`

I don't see a reason why if your running N tendermint chains, you'd only want telemetry on a subset. Is that an important thing to support?

[ValarDragon]

Asked @jackzampolin over DM, he saw no client issue with adding a global variable to the telemetry package, thats set on telemetry initialization. So lets go with that approach! (It has the easier API)

[tac0turtle]

can we do what Tendermint does and pass a noop telemetry? or this is about also avoiding time calls?

[ValarDragon]

Also avoiding the time.Now() calls.

[alexanderbez]

can we do what Tendermint does and pass a noop telemetry? or this is about also avoiding time calls?

Yes

Also avoiding the time.Now() calls.

Yes