[libbeat]MapStr/Conv toInteger should behave different when converting from nil

[odacremolbap]

Issue originating at metricbeat

• Version: 6.x, 7.x
• Operating System: Any
• Related issue: [Metricbeat] consumer_utilisation issue with RabbitMQ module in 7.0.0 #11833

schema/mapstriface conversion utility toInteger func returns:

• the integer if the conversion was possible
• a KeyNotFound error when the key is not present
• a generic error when the conversion fails

When using these conversion functions we will be considering an error a key that exist but whose value is nil, while some apps will report keys with empty values that should be treated as NotFound

Solution 1
turn this:

beats/libbeat/common/schema/mapstriface/mapstriface.go

Lines 205 to 208 in 4eb0002

	emptyIface, err := common.MapStr(data).GetValue(key)
	if err != nil {
	return 0, schema.NewKeyNotFoundError(key)
	}

into this

	emptyIface, err := common.MapStr(data).GetValue(key)
	if err != nil || emptyIface == nil {
		return 0, schema.NewKeyNotFoundError(key)
	}

it is a very simple solution that should not modify any current usage of the int conversion, but it returns a KeyNotFound error when in fact the key might have been found, but the value is nil. The result is ok, the semantics are twisted.

Solution 2
Create an NilValueError that resembles KeyNotFound

beats/libbeat/common/schema/error.go

Line 45 in 4eb0002

type KeyNotFoundError struct {

this sounds like a much better solution, although looking at KeyNotFound implementation, this rings overcomplicated (is Optional and Required needed for an error object?). The reason for having these error fields is probably a scenario where a set of keys are being processed and all errors are being retrieved and classified upon their originating schema properties (Optional, Required), and these fields are "propagated" from the schema. If that's the case, I would suggest a refactor to decouple, simplify error management and make it more intuitive: errors are not optional or required, probably they fall into 2 categories: (real) errors or something to log at most. If that fits the logic of this conversion functions, those fields could be removed, and only real errors be returned.

For sure I'll be missing many scenarios here, I'm only proposing the 2 solutions above as starting points. Personally, I would go Solution 1 (although I don't like it) to solve current issues ASAP. Then think if creating a NilValueError is worth it, and if we should refactor or not

[jsoriano]

Thanks for investigating this and exploring possible solutions. On what apps are you finding nil values?

I think current behaviour is "correct" because actually a nil is not an integer, but I agree that we should handle this case.

We should decide what behaviour we want in case of nil before choosing a solution, I see these options:

• Keep it as is, interpreting this as "wrong format", we could maybe rename this to a more general InvalidValueError.
• Return KeyNotFound error, as proposed in Solution 1.
• Ignore nil values.
• Add a (Nullable?) option to declare that a value can be nil, and then use nil as valid value (or allow to set a default). Instead of an option this could also be a wrapper over existing converters.

Any solution we decide should be implemented for all types, not only for integers.

I'd be slightly in favour of adding an option to allow nil values, this would be backwards compatible and it would be the same for any type. It would also allow to have different behaviours for different services, as nil won't mean the same in all places.

In principle I wouldn't add a NilValueError, I see it as an special case of WrongFormatError, and not sure if there would be an special handling for it.

The reason for having these error fields is probably a scenario where a set of keys are being processed and all errors are being retrieved and classified upon their originating schema properties (Optional, Required), and these fields are "propagated" from the schema. If that's the case, I would suggest a refactor to decouple, simplify error management and make it more intuitive: errors are not optional or required, probably they fall into 2 categories: (real) errors or something to log at most. If that fits the logic of this conversion functions, those fields could be removed, and only real errors be returned.

You can see more about the last refactors around here in this PR: #7335

But yes, these values are mean to classify or discard errors. I'd keep this "propagation" by now, I agree that it could maybe better to propagate the options from top to bottom and don't generate the errors if not needed, but this would require almost a full rewrite of this, not sure if it worths it. I also agree that it is not the error what is optional or required but the key. We could make the Key method of the KeyError interface to return a struct with the name and the options of the key instead of just the name as is done now. But I think we can keep this separated from the decision on what to do with nil values.

[odacremolbap]

related issue is: #11833

I would avoid keeping this as is, it is up to the reporter app to decide if my_field: 0 is the same as my_field: (<-- empty).

the simplest solution would be to create an OnErrorSetToNilValueas an ApplyOption. The caveat is that this would put into the same basket my_field: (<-- empty) which is the current scenario, and my_integer_field: "I'm a string", both set to 0.

That would be an easy fix for the issue, and I think I can live with the caveat above.

WDYT?

[jsoriano]

Adding this as an option sounds good to me 👍

[odacremolbap]

closing after #12089 got merged