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

Tag guard #1625

Merged
merged 25 commits into from
Dec 8, 2023
Merged

Tag guard #1625

merged 25 commits into from
Dec 8, 2023

Conversation

EddeCCC
Copy link
Member

@EddeCCC EddeCCC commented Nov 8, 2023

Add TagGuard, which enables to limit all or specific tags in measurements


This change is Reviewable

@EddeCCC EddeCCC changed the title Tag guard tag guard Nov 8, 2023
@EddeCCC EddeCCC changed the title tag guard Tag guard Nov 8, 2023
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #1625 (7c5ec13) into master (b204add) will increase coverage by 0.16%.
The diff coverage is 87.85%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1625      +/-   ##
============================================
+ Coverage     78.38%   78.54%   +0.16%     
- Complexity     2520     2570      +50     
============================================
  Files           257      266       +9     
  Lines          8257     8435     +178     
  Branches        984      995      +11     
============================================
+ Hits           6472     6625     +153     
- Misses         1358     1376      +18     
- Partials        427      434       +7     
Files Coverage Δ
...ocelot/commons/models/health/AgentHealthState.java 100.00% <100.00%> (ø)
...t/ocelot/config/model/metrics/MetricsSettings.java 50.00% <ø> (ø)
...l/metrics/definition/MetricDefinitionSettings.java 100.00% <ø> (ø)
...core/command/handler/impl/LogsCommandExecutor.java 85.00% <ø> (ø)
...elot/core/exporter/OtlpMetricsExporterService.java 70.49% <ø> (ø)
...tation/config/MethodHookConfigurationResolver.java 96.18% <100.00%> (ø)
...core/instrumentation/hook/MethodHookGenerator.java 85.95% <100.00%> (ø)
.../instrumentation/hook/actions/MetricsRecorder.java 100.00% <100.00%> (ø)
...n/hook/actions/span/ContinueOrStartSpanAction.java 95.56% <ø> (ø)
...n/hook/actions/span/WriteSpanAttributesAction.java 100.00% <ø> (ø)
... and 16 more

... and 1 file with indirect coverage changes

@EddeCCC EddeCCC requested review from TitusLabs and Dimi-Ma November 30, 2023 12:58
TitusLabs
TitusLabs previously approved these changes Dec 6, 2023
Copy link
Contributor

@TitusLabs TitusLabs left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 9 files at r1, 8 of 23 files at r2, 7 of 255 files at r3, 223 of 240 files at r4, 1 of 2 files at r6, 33 of 35 files at r7, 5 of 5 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Dimi-Ma)

Copy link
Member

@Dimi-Ma Dimi-Ma left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 9 files at r1, 8 of 23 files at r2, 7 of 255 files at r3, 229 of 240 files at r4, 1 of 2 files at r6, 33 of 35 files at r7, 5 of 5 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @EddeCCC)


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/ExporterServiceIntegrationTestBase.java line 330 at r7 (raw file):

        }

        public List<ExportMetricsServiceRequest> getMetricRequests() {

This function seems to not be used and could be removed


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/exporter/ExporterServiceIntegrationTestBase.java line 373 at r7 (raw file):

    }

    public OtlpGrpcServer getGrpcServer() {

same as above


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/selfmonitoring/AgentHealthIncidentBufferTest.java line 39 at r8 (raw file):

    }
    @Test
    void verifyBufferSize() {

this test is redundant.


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/selfmonitoring/AgentHealthManagerDeadlockGh1597IntTest.java line 52 at r7 (raw file):

            long start = System.currentTimeMillis();
            while (System.currentTimeMillis() - start < millisToRun) {
                cut.invalidateIncident(cut.getClass(), "Invalidation due to invalidator event");

Maybe it would be better to use a more descriptive message here.


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/selfmonitoring/AgentHealthManagerTest.java line 41 at r7 (raw file):

        @Test
        void verifyAgentHealthChangedEvent() {

This Test exists 2 times with the same name, one with invalidator as null and one where its set. Do these tests show the same result? The other test is under the nested class TimeoutHealth if its supposed to show something different it is not apparent from how the test is written. (goes for the other tests too since there are some duplicates with only the `ìnvalidator`` being a different value but having the same test name.

Copy link
Member

@Dimi-Ma Dimi-Ma left a comment

Choose a reason for hiding this comment

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

Reviewed 65 of 65 files at r9, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EddeCCC)


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/selfmonitoring/AgentHealthIncidentBufferTest.java line 39 at r8 (raw file):

Previously, Dimi-Ma (Dimitiros Mantas) wrote…

this test is redundant.

Expected value is mocked so the test case is not valid and can be removed.


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/selfmonitoring/AgentHealthManagerTest.java line 41 at r7 (raw file):

Previously, Dimi-Ma (Dimitiros Mantas) wrote…

This Test exists 2 times with the same name, one with invalidator as null and one where its set. Do these tests show the same result? The other test is under the nested class TimeoutHealth if its supposed to show something different it is not apparent from how the test is written. (goes for the other tests too since there are some duplicates with only the `ìnvalidator`` being a different value but having the same test name.

The changes are fine but the naming can still be improved. It would be better to have a distinct name for each test to represent what is being tested in it.

Dimi-Ma
Dimi-Ma previously approved these changes Dec 8, 2023
Copy link
Member

@Dimi-Ma Dimi-Ma left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EddeCCC)


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/span/WriteSpanAttributesAction.java line 4 at r10 (raw file):

import io.opentelemetry.api.trace.Span;
import lombok.*;

Do we want to group together imports of the same library if so when should be the the trigger set?


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/hook/MethodHookGeneratorTest.java line 30 at r10 (raw file):

import rocks.inspectit.ocelot.core.testutils.Dummy;

import java.util.*;

Same here, is this grouping really needed?

@EddeCCC
Copy link
Member Author

EddeCCC commented Dec 8, 2023

Pull-Requested was approved. By-Passing to avoid unnecessary delay.

@EddeCCC EddeCCC merged commit 36a8732 into master Dec 8, 2023
15 checks passed
@EddeCCC EddeCCC deleted the tag-guard branch February 1, 2024 09:50
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.

3 participants