[prior art] sdk/log/xlog: Add FilterProcessor and EnabledParameters

[pellared]

This introduces a new go.opentelemetry.io/otel/sdk/log/xlog module.
I created it because of two reasons:

• Add the possibility to filter based on the resource and scope which is available for the SDK. The scope information is the most important as it gives the possibility to e.g. filter out logs emitted for a given logger. Thus e.g. Add min_severity to LoggerConfig opentelemetry-specification#4364 is not necessary. See OTEP: Logger.Enabled opentelemetry-specification#4290 (comment) for more context.
• I think we may want to stabilize the Logs SDK before Add Enabled to LogRecordProcessor opentelemetry-specification#4363 is done and stabilized. Therefore, this feature is added a separate module. go.opentelemetry.io/otel/sdk/log module does not expose these experimental feature in its exported API; it is only available in docs and example test. The user would have to explicitly opt-in by using the go.opentelemetry.io/otel/sdk/log/xlog module which will always have v0.x.y versions. Hopefully at some point of time the features are added to go.opentelemetry.io/otel/sdk/log, deprecated in go.opentelemetry.io/otel/sdk/log/xlog and afterwards removed from go.opentelemetry.io/otel/sdk/log/xlog . I think that for at one release we would need to support both the deprecated go.opentelemetry.io/otel/sdk/xlog.FilterLogger and new go.opentelemetry.io/otel/sdk/log.FilterLogger to facilitate the migration process. The module name is inspired by xerrors. xreceiver has a similar design. xlog is under sdk/log so that it has access to packages under sdk/log/internal.
• This is also related to Document strategy to support experimental methods on stable interfaces #5882 (comment)
• This will also help me with working towards Add Enabled to LogRecordProcessor opentelemetry-specification#4363 as I would have a good example how we do support it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (b25505b) to head (5cf1eaf).

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #6271     +/-   ##
=======================================
- Coverage   81.8%   81.8%   -0.1%     
=======================================
  Files        283     283             
  Lines      24892   24900      +8     
=======================================
+ Hits       20381   20387      +6     
- Misses      4107    4109      +2     
  Partials     404     404             

see 4 files with indirect coverage changes

[dashpole]

This seems like a good improvement over the previous approach. I don't see any drawbacks, and would be comfortable using this approach elsewhere

[dmathieu]

I like this approach. This is also how the collector does experimental changes in stable packages, for example with xreceiver.

[pellared]

I like this approach. This is also how the collector does experimental changes in stable packages, for example with xreceiver.

I see that xreceiver is under receiver. Maybe it is a good thing to do as it gives the xreceiver access to receiver/internal. Does my reasoning make sense? If so then probably the module should be sdk/log/xlog.

[dmathieu]

That definitely makes sense.

[pellared]

That definitely makes sense.

Done 4e54827

[MrAlias]

Doesn't this mean when this is stabilized, this module will depend on an unstable module we control?

[pellared]

Yes, but I think the impact would be acceptable.

This would require the users to use versions that are compatible. This would affect mostly users that opt-in to use xlog so that it is a direct dependency. FWIW most people do not bump indirect dependencies.

Personaly, I think it is an acceptable trade-off.

Similarly we depend on unstable github.com/google/go-cmp and golang.org/x/sys and I do not thing it prevents as from stabilization of the modules that use it.

I think it is good to not depend on unstable modules but it is not always feasible.

[MrAlias]

Similarly we depend on unstable github.com/google/go-cmp and golang.org/x/sys and I do not thing it prevents as from stabilization of the modules that use it.

These are not maintained by us though.

[MrAlias]

This would require the users to use versions that are compatible. This would affect mostly users that opt-in to use xlog so that it is a direct dependency. FWIW most people do not bump indirect dependencies.

I think this is the crux of the problem. We need to provide stability guarantees for the SDK we release. I don't think saying "people do not bump indirect dependencies" is a valid way to get around this.

[MrAlias]

This seems like it will share the same issue we had with cross-module internal dependencies, right?

[pellared]

I am not sure if this will work with type assertions when the argument is an experiental type. But I will double check.

If not then I think I can make a longer list of arguments for Enabled (context, severity, scope, resource) instead of EnabledPararamters (which could be introduced when the feature is going to be stabilized).

I will try this in a separate PR(s) to have a comparison between different designs.

Thanks a lot for your feedback 🙏

[XSAM]

If not then I think I can make a longer list of arguments for Enabled (context, severity, scope, resource) instead of EnabledPararamters (which could be introduced when the feature is going to be stabilized).

Are you suggesting we are going to use the same EnabledParameters for Logger.Enabled and FilterProcessor.Enabled? This will lead us to an awkward position if we need to change FilterProcessor.Enabled after stabilizing the Logger.

I think we can copy experimental interface definitions to the stable module if we want to make it stable. After that, we drop the use of experimental interfaces in SDK. This will break all existing processors that implement the experimental interfaces, but it seems fine, as they are experimental features.

[pellared]

Are you suggesting we are going to use the same EnabledParameters for Logger.Enabled and FilterProcessor.Enabled?

No, we need scope.Instrumentation and resource.Resource. I meant something like

type FiltererProcessor interface {
   Enabled(context.Context, log.Severity, scope.Instrumentation, resource.Resource) bool
}

[pellared]

We can use tooling to copy experimental interface definitions to the stable module if we want to ensure syncing, but not having the to-be-stable module depend on the unstable module prevents a set of user bugs and is preferential.

As suspected. It does not work with type assertions. However, I think we can try use reflect instead to make it working.

• sdk/log/xlog: Add FilterProcessor and EnabledParameters #6286

The benefit of this approach is that it should never break the compilation for the users. It mitigates the problem if they happen to run in a conflict and have incompatible version of xlog (e.g. transient dependencies).

The main drawback for users is no compile-time guarantee for people that opt-in in to an experimental feature.
The other drawback is the complexity of the implementation (on our side) because of using reflection.
The minor drawbacks are a little more overhead

It seems reasonable to favor stability guarantees. It also looks like a safer, less disruptive approach for our users. For sure it is worth creating a PR that tries doing it so that we can compare both approaches.

The next alternative is to have an experimental interface like

type FiltererProcessor interface {
   Enabled(context.Context, log.Severity, scope.Instrumentation, resource.Resource) bool
}

This way we would be able to use type assertions, but I think it is a bad idea given we would like to introduce EnabledParameters. It would make migration from experimental to stable very awkward and inconvenient. I do not like this and I do not want to go forward. I think this would look very hacky and unprofessional. At last we would not be able to use such approach for more complex parameters.

[pellared]

#6286 is ready for review.

[dmathieu]

From the SIG meeting: could we add a check (maybe a lint) to ensure no SDK uses an x module?
The reasoning is that if we make a breaking change to an x module which is used by the SDK, folks have to upgrade everything in lockstep.

If we ensure no stable SDK uses the x modules, the only folks who have to upgrade in lockstep are folks who made the conscious choice to use an unstable module.

[XSAM]

I like the current approach. It looks promising.

[XSAM]

If not then I think I can make a longer list of arguments for Enabled (context, severity, scope, resource) instead of EnabledPararamters (which could be introduced when the feature is going to be stabilized).

Are you suggesting we are going to use the same EnabledParameters for Logger.Enabled and FilterProcessor.Enabled? This will lead us to an awkward position if we need to change FilterProcessor.Enabled after stabilizing the Logger.

I think we can copy experimental interface definitions to the stable module if we want to make it stable. After that, we drop the use of experimental interfaces in SDK. This will break all existing processors that implement the experimental interfaces, but it seems fine, as they are experimental features.

[XSAM]

From the SIG meeting: could we add a check (maybe a lint) to ensure no SDK uses an x module? The reasoning is that if we make a breaking change to an x module which is used by the SDK, folks have to upgrade everything in lockstep.

If we ensure no stable SDK uses the x modules, the only folks who have to upgrade in lockstep are folks who made the conscious choice to use an unstable module.

This PR already makes SDK to use an x module to make experimental features happen. Unless we use a shared type (like shared EnabledParameters, maybe it can be any or map) in SDK for x modules, I am not sure we can implement experimental features without importing them.