Alert execution repository #380

cwbriones · 2020-05-11T19:19:53Z

No description provided.

app/com/arpnetworking/metrics/portal/scheduling/JobExecutionRepository.java

app/models/internal/impl/DefaultAlertEvaluationResult.java

conf/postgresql.application.conf

cwbriones · 2020-05-11T19:24:37Z

...java/com/arpnetworking/metrics/portal/integration/repositories/JobExecutionRepositoryIT.java

+ *
+ * @author Christian Briones (cbriones at dropbox dot com)
+ */
+public abstract class JobExecutionRepositoryIT<T> {


This is almost exactly what's in ReportExecutionRepositoryIT. You may just want to git diff against that.

I'm going to follow up this diff with a change to make that code use this instead, but the gist of it is setUp had to be fixed to be generic and the success tests needed a newResult callback since the result type became generic.

app/com/arpnetworking/metrics/portal/alerts/impl/DatabaseAlertExecutionRepository.java

vjkoskela · 2020-05-12T02:55:29Z

app/com/arpnetworking/metrics/portal/alerts/impl/DatabaseAlertExecutionRepository.java

+        final Optional<AlertExecution> existingExecution = org.flatMap(r ->
+                _ebeanServer.createQuery(AlertExecution.class)
+                        .where()
+                        .eq("organization", org.get())


question: here we compare the organization, while below we compare the organization uuid. Is this difference necessary? Do the generated queries differ (e.g. one uses a join and one does not)? Is one better?

As far as I can tell it's identical - Ebean is smart enough to create the same query. Regardless, I'll change this to be consistent with the code below.

app/com/arpnetworking/metrics/portal/alerts/impl/NoAlertExecutionRepository.java

vjkoskela · 2020-05-12T02:58:48Z

app/com/arpnetworking/metrics/portal/scheduling/JobExecutionRepository.java

     * @return The last successful execution.
+     * @throws NoSuchElementException if no job has the given UUID.


Seems odd that we would throw this exception given that we return an Optional.

This was preserved from the old Job Repository API. I think the intent is to distinguish between executions that do not exist and parent entities that do not exist.

Optional.of(execution) - That job was valid and that execution existed

Optional.empty() - That job was valid but no execution was scheduled for that time

NoSuchElementException - That job does not exist OR that organization does not exist

If you have not already, please document this in the Javadoc for each method (I suppose in the interface).

app/com/arpnetworking/metrics/portal/scheduling/JobExecutionRepository.java

app/models/internal/impl/DefaultAlertEvaluationResult.java

conf/db/migration/metrics_portal_ddl/V21__create_alert_execution_tables.sql

app/com/arpnetworking/metrics/portal/alerts/impl/DatabaseAlertExecutionRepository.java

app/models/internal/impl/DefaultAlertEvaluationResult.java

vjkoskela · 2020-05-12T03:17:02Z

app/models/ebean/AlertExecution.java

+    @Column(name = "alert_id")
+    private UUID alertId;
+    @Nullable
+    @DbJsonB


This is your likely serialization culprit.

Here's the relevant thread:

ebean-orm/ebean#542

This change related to 542 may offer some help but I have not traced it all the way down:

ebean-orm/ebean@13f7d37#diff-2a7fe1eac4aafd91195da026c9a88a9fR675

Specifically, here it looks into ServerConfig for an ObjectMapper, if we can set that before this method is called to the one we created in Metrics Portal then you would get all the goodies we are used to having with ObjectMapper usage.

If not, the fallback strategy here imo is to not use @DbJsonB and instead perform the serialization/deserialization in the repository object. The only downfall of this imo is that the schema may not be easily upgradable to @DbJsonB in the future (unless you can find a type that works today with manual serde and in the future with Ebean serde).

Thanks for the pointers!

I went ahead and configured this in both MainModule and EbeanServerHelper. It looks like it did the trick.

Hmm. Apparently it works fine in the unit tests, but I'm actually discarding configuration when trying to use it for integration tests because I don't load the same configuration that play does using the hocon file.

I'm still trying to figure out how to override ServerConfig from within Play. If this takes more than an hour I'll probably go with the fallback strategy mentioned.

I'm a little confused by what you mean when you say integration tests discard the configuration.

There are two styles of "integration" tests.

*IT.java code is executed in the Maven JVM against Metrics Portal JVM (e.g. via its APIs) running in Docker. The configuration loaded by Metrics Portal is the one packaged by default into the Docker image.

*IT.java code is executed in the Maven JVM and loads and runs classes from Metrics Portal against actual dependencies (e.g. Postgresql, Cassandra, KairosDb). There is no configuration in this context in the Maven JVM (nor should there be).

Instead, you will need to have the tests inject the correct ObjectMapper. You need to be careful because you want all Ebean tests to inject a single and consistent ObjectMapper instance.

We actually have a similar problem for working with the Play APIs because they use (the same) custom ObjectMapper. The code for generating this for tests is here:

https://github.com/ArpNetworking/metrics-portal/blob/master/test/java/com/arpnetworking/testing/SerializationTestUtils.java

You should probably re-use it with some way for Ebean tests (e.g. BaseEbean abstract test class with @BeforeClass that does the injection). Shoot me a text and we can talk specifics.

These integration tests are the second flavor (only one JVM, no Play APIs, "real" external databases). I understand that there are two kinds but given the context of the PR I thought it was reasonable to omit any specifics.

What I had meant by "discard configuration" is that because of the way this is now set up to configure the ServerConfig, this MetricsPortalServerConfigStartup class needs to be loaded by ebean in order to call the callback and populate the ObjectMapper field. As mentioned below, it has to be called at inject time otherewise the ObjectMapper is null and everything fails.

In the tests, we load the ebean classes manually from within EbeanServerHelper. This sets the ObjectMapper to something reasonable, but if MetricsPortalServerConfigStartup gets loaded here, it will clobber the ObjectMapper. This scenario is what I mean by "discard" - we overwrite any configured value. I was accidentally doing this in one of my earlier commits and it caused my tests to fail.

It sounds like then there are currently 2 (3?) locations where we create an object mapper, and they all vary independently (even though they should all be the same):

MainModule

EbeanServerHelper

SerializationTestUtils

At the very least I can make the last two the same since they're both run during tests.

Yes please! Reducing the two in tests to just one would be very helpful. The duplication and matching with prod is already called out here:

https://github.com/ArpNetworking/metrics-portal/blob/master/test/java/com/arpnetworking/testing/SerializationTestUtils.java#L40

Feel free to file an issue to refactor the Prod code so it can be used from Test as well. That would make it just one definition. Although to be clear, the risk there is someone modifying the prod definition and the tests passing BUT not being backwards compatible. The dual definition should at least catch such configuration changes to ObjectMapper.

So 2 is better than 3, but 1 may not be better than 2. ;)

This was being loaded during test start.

vjkoskela · 2020-05-13T20:40:21Z

app/com/arpnetworking/metrics/portal/scheduling/impl/DatabaseExecutionHelper.java

@@ -73,6 +73,8 @@ public DatabaseExecutionHelper(
     * @return An internal model for this execution
     */
    public static <T, E extends BaseExecution<T>> JobExecution<T> toInternalModel(final E beanModel) {
+        // TODO(cbriones) - This should not be static, repositories should call this from an instance.


If this is not in your development plan to address with Alerting, please file an issue for it against the OSS project.

vjkoskela · 2020-05-13T20:41:11Z

app/global/MetricsPortalServerConfigStartup.java

+/**
+ * Plugin class to configure Ebean's {@link ServerConfig} at runtime.
+ * <p>
+ * This is necessary for configuring the Ebean server with dependencies constructed via Guice, for instance. This


nit: try to wrap comments at 80 characters.

After seeing this I thought I could just set this option in IntelliJ. Unfortunately not only can you not set different margins for comments v. code, the ticket for this was opened 7 years ago.

In fact, another was opened 15(!!!) years ago

I'll just do this manually...

app/global/MetricsPortalServerConfigStartup.java

vjkoskela · 2020-05-13T20:46:12Z

app/global/MetricsPortalServerConfigStartup.java

+            // In some cases we manually load the ebean model classes via ServerConfig#addPackage (see EbeanServerHelper).
+            //
+            // If this class is accidentally loaded in those environments, then injection won't occur and we'll silently overwrite
+            // the configured ObjectMapper with null. Explicitly throwing makes this error appear obvious.


I believe in either case (setting it to null or leaving it alone) this mean that the ObjectMapper is left as its default instantiated value, such that any code relying on the custom behavior will fail in weird ways. In production this could even cause data corruption (due to serializing this incorrectly). Right?

Should we be more draconian here and say System.exit()?

I don't think that's necessary - this exception seems to kill the process on startup because Guice fails to instantiate the repository. The dependency chain looks like:

DatabaseXXXRepository -> EbeanServer -> ObjectMapper -> **Boom**

^ Please add that to the comment (or something to that effect)

vjkoskela

Looks good. Just a few questions.

Introduce a DailyPartition manager actor that runs within the DatabaseAlertExecutionRepository, periodically attempting to create partitions for the alert executions table that was added in #380. Notably I've introduced another EbeanServer which uses the DDL role (metrics_dba), since that Role is necessary to create tables. I've included unit tests for the actor while the DB testing is covered by the existing integration tests.

cwbriones added 6 commits May 7, 2020 19:26

alert execution repository

e8b7a7f

tests pass

36f9eeb

create a partition for the current date on initial migration

289042a

generic integration tests

fc9b2a9

temporarily revery ReportExecutionRepository test, doc comments

7103d06

lint

980b16f

cwbriones commented May 11, 2020

View reviewed changes

app/com/arpnetworking/metrics/portal/scheduling/JobExecutionRepository.java Outdated Show resolved Hide resolved

cwbriones commented May 11, 2020

View reviewed changes

app/models/internal/impl/DefaultAlertEvaluationResult.java Outdated Show resolved Hide resolved

cwbriones commented May 11, 2020

View reviewed changes

conf/postgresql.application.conf Show resolved Hide resolved

cwbriones commented May 11, 2020

View reviewed changes

cwbriones and others added 4 commits May 11, 2020 12:26

Merge branch 'master' into alert-execution-repository

5f8ef14

fix broken doc link

3cc1734

initial fixes noticed after opening PR

93e4ae7

Merge branch 'master' into alert-execution-repository

3364484

vjkoskela requested changes May 12, 2020

View reviewed changes

cwbriones added 6 commits May 12, 2020 15:25

address comments

b1505bb

merge master

879cc4f

fix rebase conflict with AlertEvaluationResult

e832b5b

hook into Ebean configuration using ServerConfigStartup

89f9dea

move the ebean startup outside of models.ebean

681cee6

This was being loaded during test start.

write more explanatory comments, throw on misuse

9cfb16c

vjkoskela reviewed May 13, 2020

View reviewed changes

app/global/MetricsPortalServerConfigStartup.java Outdated Show resolved Hide resolved

vjkoskela reviewed May 13, 2020

View reviewed changes

vjkoskela approved these changes May 13, 2020

View reviewed changes

cwbriones added 3 commits May 13, 2020 15:26

tweak language, wrap comments

8bdc493

use the API object mapper for EbeanServerHelper

1bb0b7b

elaborate on possibility of prod data corruption

08b7b78

cwbriones merged commit 4de7284 into ArpNetworking:master May 14, 2020

cwbriones mentioned this pull request May 15, 2020

refactor report execution test #385

Merged

cwbriones deleted the alert-execution-repository branch June 3, 2020 20:20

cwbriones mentioned this pull request Jun 17, 2020

Periodically create partitions for alert executions #398

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alert execution repository #380

Alert execution repository #380

cwbriones commented May 11, 2020

cwbriones May 11, 2020

vjkoskela May 12, 2020

cwbriones May 12, 2020

vjkoskela May 12, 2020

cwbriones May 12, 2020

vjkoskela May 13, 2020

vjkoskela May 12, 2020

cwbriones May 12, 2020

cwbriones May 12, 2020

vjkoskela May 13, 2020

cwbriones May 13, 2020

vjkoskela May 14, 2020

vjkoskela May 13, 2020

cwbriones May 13, 2020

vjkoskela May 13, 2020

cwbriones May 13, 2020

vjkoskela May 13, 2020

cwbriones May 13, 2020

vjkoskela May 14, 2020

vjkoskela left a comment

		* @return The last successful execution.
		* @throws NoSuchElementException if no job has the given UUID.

Alert execution repository #380

Alert execution repository #380

Conversation

cwbriones commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjkoskela left a comment

Choose a reason for hiding this comment