Skip to content
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

Add reaper option to keep all keys #995

Merged
merged 1 commit into from
Apr 17, 2021

Conversation

mreinhardt
Copy link
Contributor

No description provided.

@mreinhardt
Copy link
Contributor Author

Just to note: I have a practical production use-case that would benefit greatly from this instead of constantly having to update keep-keys. We have a post-expiry fn that will take care of anything that gets carried over that may need to change (like ttl for possible re-indexing), but it's cumbersome to have to keep track of required keys and some folks have been missing expiry alerts until we realize that one of those keys got ditched and the event gets filtered out due to it. Thanks for considering, willing to make any required changes to get this into master.

@jamtur01
Copy link
Member

@aphyr Any thoughts?

@aphyr
Copy link
Collaborator

aphyr commented Apr 16, 2021

I... think this is OK. It's honestly been so long since I've been in Riemann's guts that I've lost my intuition for all the ways event expiry interacts with various streams, but I think a.) this behavior is opt-in and b.) it's clearly explained, so... it should be... okay? If things break, it's On You, haha. :-)

What I'd worry about here on a theoretical basis is that keep-keys sort of blurs the boundary between services being uniquely identified by [host service] and being identified by any other field. If you need to know information about what the most recent state was before expiry, that's nominally something you'd keep track of in the stream itself. For instance, while people may use tags to identify a service over time, you might get two events for the same service with tags, say, ["foo"] and ["bar"]. Is that service tagged ["foo"] still? Tags are, in Riemann's model, an observable value, not an identifying coordinate, which is why they're (nominally) omitted from expiry.

However, I recognize that that for all kinds of reasons your filtering might rely on other fields than host and service--event expiry has never been clean, which is, IIRC, why we added keep-keys in the first place. And in that spirit, I think adding an :all here is a reasonable and logical extension.

It makes me wonder whether it might be worth adding a special field like :last-event to expiry events, so that you could recover the full context in every case.

TL;DR: Riemann expiry has rough edges, we should give people tools to override that behavior in their best judgement, I think this is OK. :-)

@jamtur01 jamtur01 self-assigned this Apr 16, 2021
@mreinhardt
Copy link
Contributor Author

mreinhardt commented Apr 16, 2021

Thanks James and Kyle. Like I said we have a practical use case for this. We have a pretty open system and people have various keys that they may or may not need. in this case, someone was missing a heartbeat failure alert because one of the keys that he needed (:env "prod") was not in keep-keys so the expired event was being filtered out of his stream. I don't want to keep track of what everyone may or may not need, so if we keep all keys and I have a post expiry function that massages things accordingly, this will work for us. Thanks again.

@jamtur01
Copy link
Member

Alright let's merge this and we'll see how we go. Thanks @mreinhardt and @aphyr.

@jamtur01 jamtur01 merged commit f20e989 into riemann:master Apr 17, 2021
@mreinhardt mreinhardt deleted the reaper-keep-all-keys-option branch April 17, 2021 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants