[events/service checks] Allow Integer date_happened / timestamp option #115
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When formatting events, we process all opts as if they're Strings, but date_happened is actually an Integer timestamp. Added logic and unit tests to handle this Integer option.
|
I think we should still accept strings, to not break backwards compatibility... if someone was passing a string before, this would break it for them. The bug report says that they ended up passing a string, for example. Apart from that, LGTM 😃 |
|
Should we do the same for |
KSerrania
force-pushed
the
kserrania/fix-date-happened-event
branch
from
6a55ffb to
1bf7e1e
Compare
|
Yes, that would make sense. |
KSerrania
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
When formatting events, we process all opts as if they're
Strings,but
date_happenedis actually anIntegertimestamp.Added logic and unit tests to handle this
Integeroption.This fixes issue #100.
Additional notes
I'm not sure if it's better to log a warning and ignore the option, raise an exception or do nothing and add the option in the event string whatever the value of
date_happenedis.Should we also type check the
Stringoptions? That could break some non-conventional use cases (for example, right now passing an array in thehostnameoption won't crash the client but shouldn't be accepted).Thoughts @albertvaka?