Should remap emit a warning if we del a field that doesn't exist?

[StephenWakely]

If in Remap you run del on a field that doesn't exist it silently does nothing.

Would it be worth emitting a warning if this occurs?

The chances are that the user has made an error if this happens and things may not be working as expected, but they won't be able to identify the problem. Emitting a warning would help with the debugging.

There are people who may want to del even if the field doesn't exist and will not want the warning. For those users, they could wrap the call in an exists to supress the unneccessary warning.

[lucperkins]

Throwing an error by default seems overly aggressive here (just a gut feeling), but perhaps that could be enabled via an optional arg:

del(.my_field, err_if_missing = true)

[binarylogic]

I like @lucperkins suggestion, or even a del! variant (Ruby!). At most, a debug message should be logged, but it's fine if nothing is logged. The outcome is the same.

[JeanMertz]

Or, we could have del no longer return null, but have it return a boolean, indicating if the field was deleted. That keeps the function signature simple, and allows the user to optionally act on the failure to delete a field.

[JeanMertz]

You could error yourself in that case:

assert(del(.foo), "failed to delete .foo")

Or log:

if !del(.foo) {
	log("failed to delete .foo", level = "warn")
}

Or whatever else you want to do.

[spencergilbert]

For what it's worth, the old transform logged it as a debug message: https://github.com/timberio/vector/blob/master/src/internal_events/remove_fields.rs#L20

[JeanMertz]

For what it's worth, the old transform logged it as a debug message

Good point. I would still be favouring TRL not logging anything by default (the remap transform can still log if the program failed, of course), but allow the user to use log wherever they want to.

[jszwedko]

Or, we could have del no longer return null, but have it return a boolean, indicating if the field was deleted. That keeps the function signature simple, and allows the user to optionally act on the failure to delete a field.

Just noting that this won't allow differentiation between undefined and defined, but with a value of null.

[JeanMertz]

I’m not sure I understand what you mean @jszwedko.

I think the confusion might come from the fact that you think the del function returns the value of the field that you delete. This is not the case, it always returns null.

However, your comment reminds me that we did decide to update this function to take only a single path and return the value of the path that was deleted, and I still think that is a desireable property to have.

So I retract my proposal, because in that case, indeed, we can’t have it return a boolean value.

I also realize now that the Object API is allowed to return an error when you ask it to delete a path. And in fact, when you try to delete an “undeletable path” for metrics events (e.g. .tags), it will return an error and thus the del function itself is fallible as well (unless of course we decide to ignore or only log such an error, but I’m not sure that’s desireable).

So given this, I’m almost inclined to say: when deleting a path, you have to handle the error case for when a path does not exist.

For example:

.timestamp = to_timestamp(del(.ts), default = now())

// or

if ok($json = parse_json(del(.message))) {
    . = $json
} else {
    .error = “unable to parse message field”
}

// or

.bytes = if ok($bytes = to_int(del(.bytes))) {
    $bytes
} else {
    0
}

// or with other error handling functions, which is orthogonal to this discussion

.bytes = ok_or(to_int(del(.bytes)), 0)

Our error handling story is still being fleshed out, so I expect any minor inconvenience this causes right now to no longer be an issue in the future.

[jszwedko]

Aha, yes, I was thinking about it returning the deleted value, but forgot that didn't happen yet.

[binarylogic]

Closing in favor of #5779