-
Notifications
You must be signed in to change notification settings - Fork 474
fix(azure.eventhub): handles properties as string and drops empty fields #14959
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(azure.eventhub): handles properties as string and drops empty fields #14959
Conversation
As azure.eventhub.properties.raw
return (((Map) object).size() == 0); | ||
} else if (object instanceof List) { | ||
((List) object).removeIf(value -> dropEmptyFields(value)); | ||
return (((List) object).length == 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be (((List) object).size() == 0)
isn't it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I thought painless is Java, but I think .length
is an addon by painless.
🚀 Benchmarks reportTo see the full report comment with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
packages/azure/data_stream/eventhub/elasticsearch/ingest_pipeline/parsed-message.yml
Outdated
Show resolved
Hide resolved
…ine/parsed-message.yml Co-authored-by: kaiyan-sheng <[email protected]>
The sample event here has the properties with fields and the conditions property has empty string and also fields with values. Can we include this format as well to the pipeline tests? |
💚 Build Succeeded
History
cc @zmoog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment on adding additional events to the pipeline tests. Change looks good to me otherwise!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! In case it’s useful to know you can also apply subobjects: false
to solve the conflict. Done for example at #13925.
Yeah, but this would require to add mappings for these fields not to make test fail, which is something I am trying to avoid in a generic integration like |
Great point, I didn't think about using Since this is a generic integration, where users are supposed to customize pipeline and mapping, I'm trying to leave all the options on the table. |
Package azure - 1.28.3 containing this change is available at https://epr.elastic.co/package/azure/1.28.3/ |
Proposed commit message
WHAT
azure.eventhub.properties
asazure.eventhub.properties.raw
whenazure.eventhub.properties
is a string instead of an object.WHY
Drops empty/null fields
Sometimes Azure services emit log event like the following:
Elasticsearch cannot index
"": ""
field insideproperties.condition
, so we need to clean it up.Renames
azure.eventhub.properties
asazure.eventhub.properties.raw
whenazure.eventhub.properties
is a string instead of an objectSometimes Azure sends
azure.eventhub.properties
as a string, this field should really be a anobject
instead, causing mapping errors like:By renaming
azure.eventhub.properties
asazure.eventhub.properties.raw
, we avoid the mapping error and give users the opportunity to handle the original value using a custom pipeline.Checklist
changelog.yml
file.I have verified that any added dashboard complies with Kibana's Dashboard good practices