Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

For a bug fix, I would expect to see some tests that verify that the bug has been fixed. There are no changes to the tests so running the test suite only validates that the old tests did not get broken.

For a bug fix, I would expect to see some tests that verify that the bug has been fixed. There are no changes to the tests so running the test suite only validates that the old tests did not get broken.

I agree, it makes sense to add more tests to prove the change is effective.

To add more context, this is not addressing a user-reported actual occurrence of this problem. This is mostly a defense against future off-spec changes, so we don't know which data stream will receive off-spec dates yet. Azure often emits off-spec values in dates and all sort of other fields.

I classified this PR as a bugfix because Azure-focused integrations have to work in this environment.

Maybe we can add a sample log for each additional format as @StacieClark-Elastic mentioned about test?

Yeah, and I guess we need to do it for every data stream in the Azure Logs package.

Tests related to the changes would be nice. Otherwise, LGTM!

Thanks for all the comments and suggestions: collecting time format samples to add a "corner cases time formats" test file for each data stream.

@StacieClark-Elastic, added the following test case with the know time formats:

{"time": "01/09/2007 09:41:00", "category": "Administrative"}
{"time": "1/9/2007 09:41:00", "category": "Administrative"}
{"time": "01/09/2007 09:41:00 AM", "category": "Administrative"}
{"time": "1/9/2007 9:41:00 AM", "category": "Administrative"}
{"time": "1/9/2007 10:41:00 AM +01:00", "category": "Administrative"}
{"time": "2007-01-09T09:41:00", "category": "Administrative"}
{"time": "2007-01-09T09:41:00.22Z", "category": "Administrative"}
{"time": "2007-01-09T09:41:00.6816663Z", "category": "Administrative"}
{"time": "2007-01-09T09:41:00.535404056Z", "category": "Administrative"}
{"time": "2007-01-09T09:41:00.992099+00:00", "category": "Administrative"}
{"time": "2007-01-09T11:41:00+02:00", "category": "Administrative"}

Then I added the test case to all data streams that attempt to parse the @timestamp field.

This should ensure all data streams support the known data format variations we spotted in the wild. Please let me know what you think of this approach.

For a bug fix, I would expect to see some tests that verify that the bug has been fixed. There are no changes to the tests so running the test suite only validates that the old tests did not get broken.

@StacieClark-Elastic, please let me know if you think the change requires more/different tests coverage.

💚 Build Succeeded

• Buildkite Build
• Commit: 13a4b69

History

• 💚 Build #35363 succeeded 8847cb4
• 💚 Build #35257 succeeded 2b76017
• 💚 Build #35156 succeeded f64bbff
• 💚 Build #35143 succeeded 445a014

cc @zmoog

@StacieClark-Elastic, I'd appreciate your review when you get a moment! 🙇

Package azure - 1.31.1 containing this change is available at https://epr.elastic.co/package/azure/1.31.1/