feat: Ability to disable log collection at the tracer level #1254
Conversation
There was a problem hiding this comment.
@Angith I have done one round of review. Kindly check my comments. We can discuss together.
| // The main benefit of disabling is reducing the overall amount of data being collected and processed. | ||
|
|
||
| // See https://github.ibm.com/instana/technical-documentation/blob/master/tracing/specification/README.md#span-disabling for details | ||
| Disable map[string]bool |
There was a problem hiding this comment.
Suggestion: In my opinion, Disable variable name is not providing the complete context in this scenario as TracerOptions has options for multiple configuration items.
| Tracer contains tracer-specific configuration used by all tracers. Key options include: | ||
|
|
||
| - **Disable**: A map of span categories to disable. Currently, only the "logging" category is supported. See [Disabling Log Spans](disabling_spans.md) for details. | ||
| - **DropAllLogs**: Turns log events on all spans into no-ops when set to true. |
There was a problem hiding this comment.
Query: How can the user set this, DropAllLogs? Is it a redundant one? May be we can discuss.
| func (c spanCategory) String() string { | ||
| return string(c) | ||
| } | ||
|
|
There was a problem hiding this comment.
Suggestion: A comment would be nice since it is an exported function.
| // If it is, it takes precedence over agent configuration | ||
| isConfigSet := len(sensor.options.Tracer.Disable) != 0 | ||
| if isConfigSet { | ||
| r.logger.Info("Disable Tracing configuration is set either through in-code or INSTANA_TRACING_DISABLE environment variable," + |
There was a problem hiding this comment.
Suggestion: Does it make sense to split this long log statement?
| "os" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/stretchr/testify/assert/yaml" |
There was a problem hiding this comment.
I am not positive about using a testing library in a source file.
@nithinputhenveettil What do you think?
| @@ -0,0 +1,43 @@ | |||
| // (c) Copyright IBM Corp. 2025 | |||
There was a problem hiding this comment.
I think we should name the file for the component present, not for a feature.
| @@ -0,0 +1,43 @@ | |||
| // (c) Copyright IBM Corp. 2025 | |||
|
|
|||
| package instana | |||
There was a problem hiding this comment.
Query: In this implementation, do we need a separate file for spanCategory as such?
| } | ||
|
|
||
| // registeredSpanMap maps span types to their categories | ||
| var registeredSpanMap map[RegisteredSpanType]spanCategory = map[RegisteredSpanType]spanCategory{ |
There was a problem hiding this comment.
Suggestion: Just check whether a function with(switch cases in the future) will be more suiting here. I think we won't be needing a map in that case.
| if c == unknown { | ||
| return true | ||
| } | ||
| return !sensor.options.Tracer.Disable[c.String()] |
There was a problem hiding this comment.
I believe, false while accessing the map can imply either the value is not present or the map is not initialised. Just wanted to make sure that it is as per design. We can discuss.
| return unknown | ||
| } | ||
|
|
||
| func (c spanCategory) Enabled() bool { |
There was a problem hiding this comment.
Query: Won't Disabled() is more meaningful?
|
This PR enables Go Tracer to disable log spans at tracer level.
There are four ways to disable log spans: