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

Closes #923 - Added initiator beacon requirement to EUM Server #924

Merged
merged 2 commits into from
Sep 9, 2020

Conversation

JonasKunz
Copy link
Member

@JonasKunz JonasKunz commented Sep 9, 2020

Closes #923


This change is Reviewable

Copy link
Member

@mariusoe mariusoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @JonasKunz)


components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/configuration/model/BeaconRequirement.java, line 63 at r1 (raw file):

    private RequirementType requirement;

    private List<InitiatorType> initiators = Arrays.asList(InitiatorType.values());

Please add some docs to this field as well


components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/configuration/model/BeaconRequirement.java, line 67 at r1 (raw file):

    /**
     * The field which is targeted.
     * Used for the "EXISTS" and "NOT_EXISTS" requirement types.

Can you use {@link RequirementType#EXISTS} in the docs, then there is a link to the acutal enums


components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/configuration/model/InitiatorType.java, line 35 at r1 (raw file):

hasType

How about naming this isEqualTo? Imo hasType feels somehow wrong for this method.
Or just overloading the method by hasInitiator(String)


components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/configuration/model/InitiatorType.java, line 37 at r1 (raw file):

    abstract boolean hasType(String httpInitiatorValue);

    public boolean hasInitiator(Beacon beacon) {

please add a small docu


components/inspectit-ocelot-eum-server/src/test/java/rocks/inspectit/oce/eum/server/configuration/model/BeaconRequirementTest.java, line 144 at r1 (raw file):

nullInitiator

Maybe we should adjust the test namings so we don't have a mix between camel and snake casing


components/inspectit-ocelot-eum-server/src/test/java/rocks/inspectit/oce/eum/server/configuration/model/BeaconRequirementTest.java, line 153 at r1 (raw file):

            assertThat(result).isFalse();
        }
    }

It would be good to have a test for the document as well because that is a bit special with its null check.

@Test
public void documentInitiator() {
    Beacon beacon = Beacon.of(Collections.emptyMap());
    requirement.setInitiators(Collections.singletonList(InitiatorType.DOCUMENT));
    requirement.setRequirement(BeaconRequirement.RequirementType.HAS_INITIATOR);

    boolean result = requirement.validate(beacon);

    assertThat(result).isTrue();
}

inspectit-ocelot-documentation/docs/enduser-monitoring/eum-server-configuration.md, line 158 at r1 (raw file):

The following requirement types are currently be supported:

| Type | Note |

How about adding this table to the top, before the example so there is a quick overview on existing requirement types?

mariusoe
mariusoe previously approved these changes Sep 9, 2020
Copy link
Member

@mariusoe mariusoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @JonasKunz)


components/inspectit-ocelot-eum-server/src/main/java/rocks/inspectit/oce/eum/server/configuration/model/BeaconRequirement.java, line 39 at r1 (raw file):

     * The supported types of requirements.
     */
    public enum RequirementType {

Since there is no validation anymore, in my oppinion, we should add a validation due to the fact that some fields are not required in case a specific requirement type is used.
This would also prevent some errors and confused users.
Doing this, the server fails immeditaly when providing a wrong config.

 @AssertTrue
public boolean isValid() {
    switch (requirement) {
        case EXISTS:
        case NOT_EXISTS:
            return field != null && initiators == null;
        case HAS_INITIATOR:
            return field == null && initiators != null;
        default:
            log.error("Requirement of type {} is not supported.", requirement);
            return false;
    }
}

Copy link
Member

@mariusoe mariusoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@JonasKunz JonasKunz merged commit 75c306c into inspectIT:master Sep 9, 2020
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.

Add metric filterting based on beacon initiator
2 participants