-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(automated-rules): implement automated rules #34
Conversation
I used https://github.com/projectnessie/cel-java as the implementation for Google's CEL for java. Google's repo doesn't seem to support our use case for now: google/cel-java#68 but it seems the two projects are teaming up? google/cel-java#37 |
The Project Nessie implementation looks easier to build downstream: Gradle instead of Bazel. The only drawback is if this implementation merges into Google's, we'll have to start from scratch in terms of building 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.
Looking great from a read-through. I'll give this a try soon.
Signed-off-by: Max Cao <[email protected]>
Signed-off-by: Max Cao <[email protected]>
Signed-off-by: Max Cao <[email protected]>
Signed-off-by: Max Cao <[email protected]>
Signed-off-by: Max Cao <[email protected]>
Signed-off-by: Max Cao <[email protected]>
Signed-off-by: Max Cao <[email protected]>
Signed-off-by: Max Cao <[email protected]>
Signed-off-by: Max Cao <[email protected]>
28544a9
to
3c89113
Compare
I think the new uniqueness constraint on the ActiveRecording name isn't working as anticipated:
To get this I just created a rule named |
Easy fix after all that I just applied myself. This might be something to refactor and deal with later, but I thought that using quarkus-quartz to handle periodic archiving could be good. This would take the place of the |
I can try fitting it here. |
https://quarkus.io/guides/scheduler-reference#programmatic_scheduling This might be a better link to reference. I think the part about Quartz being used for persistence of scheduled tasks makes sense - that's pretty much exactly what our archiver rules are. |
I assume the Rules represent the persistent scheduled tasks themselves and used those. |
Yep exactly. |
Quartz change looks great from a reading. |
It doesn't seem that archive pruning is working - I created a rule with |
Hm.. I don't see this issue, can you give me recreation instructions? |
I'll try it again in a few minutes, I have an integration test run on a 2.3.1 backport currently running so things are a little tied up. |
Okay, I think the problem is the naming scheme of the targets and the regex pattern that determines the recording name from the recording file name. private static final Pattern RECORDING_FILENAME_PATTERN = Pattern.compile( "([A-Za-z\\d\\.-]*)_([A-Za-z\\d-_]*)_([\\d]*T[\\d]*Z)(\\.[\\d]+)?(\\.jfr)?"); The |
Right, makes sense. In the short term, sanitizing the input and applying transformations like that is fine - Cryostat has been doing that since as long as it has had archives. In the longer term I'm tempted to say let's do something much more radical. We can safely embed those details about the source target (alias, connectUrl, JVM ID, labels/annotations) into a
If we redo the tables for targets and active recordings so that we don't delete records when targets go offline or recordings are deleted, and instead add a status flag column, then we can always go back and look up the details of the source recording for an archived recording, and from there also look up the details of the source target. |
Does this affect the 2.3/2.4 codebase as well then? |
Never noticed, but seems like this was already done by Janelle 2 years ago now: String targetName =
platformClient.listDiscoverableServices().stream()
.filter(
serviceRef -> {
return serviceRef.getServiceUri().equals(serviceUri)
&& serviceRef.getAlias().isPresent();
})
.map(s -> s.getAlias().get())
.findFirst()
.orElse(connection.getHost())
.replaceAll("[\\._]+", "-"); So I assume it's safe to continue with this, It probably makes sense to change it to include periods and the backslash too. |
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.
Seems to be working very well, just a few more minor comments.
Related #15
sample test queries: