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 #1495 - improve the behavior when agent commands fail (or are not available) due to missing configuration - backend #1527

Merged
merged 10 commits into from
Oct 17, 2022

Conversation

danipaniii
Copy link
Contributor

@danipaniii danipaniii commented Sep 27, 2022

Closes #1495
Implementing improved behavior in the UI when agent commands are not enabled for the log-preloading button and the support archive button. Unless the necessary settings are not enabled for the log-preloading or the support archive, the buttons will be deactivated and in the tooltip will be a message telling the user how to enable the buttons.


This change is Reviewable

Copy link
Member

@MariusBrill MariusBrill left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 10 files at r1, all commit messages.
Reviewable status: 5 of 10 files reviewed, 9 unresolved discussions (waiting on @danipaniii and @MariusBrill)


components/inspectit-ocelot-configurationserver/src/main/java/rocks/inspectit/ocelot/agentstatus/AgentMetaInformation.java line 56 at r1 (raw file):

     *  Name of the settings states header.
     */
    private static final String HEADER_SETTING_STATES = HEADER_PREFIX + "setting-states";

I think a more befitting name would be "service-state-map". You can also add a short description on what is saved in this header.


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/config/propertysources/http/HttpPropertySourceState.java line 260 at r1 (raw file):

        httpGet.setHeader(META_HEADER_PREFIX + "START-TIME", String.valueOf(runtime.getStartTime()));
        httpGet.setHeader(META_HEADER_PREFIX + "HEALTH", agentHealth.name());
        httpGet.setHeader(META_HEADER_PREFIX + "SETTING-STATES", DynamicallyActivatableServiceObserver.getSettingStatesJSON());

Again - SERVICE-STATES-MAP =)


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/selfmonitoring/service/DynamicallyActivatableServiceObserver.java line 12 at r1 (raw file):

/**
 * Observes all the currently available services and their current state (enabled/disabled)

you can delete the "the"


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/selfmonitoring/service/DynamicallyActivatableServiceObserver.java line 14 at r1 (raw file):

 * Observes all the currently available services and their current state (enabled/disabled)
 *
 * Has a method to generate a JSON String out of the services and their states to send it to the UI

Can you elaborate the use case of this class a bit further? Which services are Observed and why? What does the JSON String resemble?


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/selfmonitoring/service/DynamicallyActivatableServiceObserver.java line 19 at r1 (raw file):

public class DynamicallyActivatableServiceObserver {
    @Getter
    private static Map<String, Boolean> serviceStates = new HashMap<>();

rename this variable to serviceStateMap


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/selfmonitoring/service/DynamicallyActivatableServiceObserver.java line 24 at r1 (raw file):

    private static String settingStatesJSON = "{}";

    public void getServices(DynamicallyActivatableService service) {

Your function name is wrong. You don't "get" a service, you save it into the serviceStateMap. So a name like "updateServiceState" would me more fitting.


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/selfmonitoring/service/DynamicallyActivatableServiceObserver.java line 27 at r1 (raw file):

        serviceStates.put(service.getName(), service.isEnabled());

        settingStatesJSON = MapToJson();

You don't need to re-generate the settingsStatesJson every time a service is updated.
Just do so in a custom written getter-method.


inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/service/DynamicallyActivatableService.java line 43 at r1 (raw file):

    private List<Expression> configDependencies;

    private DynamicallyActivatableServiceObserver serviceObserver = new DynamicallyActivatableServiceObserver();

You can make the DASObserver a @component and just autowire it here using @Autowired


inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/selfmonitoring/service/DynamicallyActivatableServiceObserverTest.java line 1 at r1 (raw file):

package rocks.inspectit.ocelot.core.selfmonitoring.service;

We already talked about the tests so far :)

Copy link
Member

@MariusBrill MariusBrill 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: 5 of 10 files reviewed, 10 unresolved discussions (waiting on @danipaniii and @MariusBrill)


components/inspectit-ocelot-configurationserver-ui/src/components/views/status/StatusTable.js line 195 at r1 (raw file):

          tooltip={logAvailable && agentCommandsEnabled ? "Show Logs" : "Please enable 'log-preloading' and 'agent-commands' in the config! Make sure to pass the right url for the agent commands!"}
          tooltipOptions={{ showDelay: 500 }}
          disabled={!logAvailable}

disabled here should be !logAvailable || !agentCommandsEnabled

@codecov
Copy link

codecov bot commented Sep 28, 2022

Codecov Report

Merging #1527 (3922ece) into master (4ae7a26) will decrease coverage by 0.84%.
The diff coverage is 12.37%.

❗ Current head 3922ece differs from pull request most recent head 74ab0ff. Consider uploading reports for the commit 74ab0ff to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1527      +/-   ##
============================================
- Coverage     78.64%   77.80%   -0.84%     
- Complexity     2271     2275       +4     
============================================
  Files           232      234       +2     
  Lines          7438     7535      +97     
  Branches        887      894       +7     
============================================
+ Hits           5849     5862      +13     
- Misses         1219     1304      +85     
+ Partials        370      369       -1     
Impacted Files Coverage Δ
.../src/main/java/com/mindprod/jarcheck/JarCheck.java 0.00% <0.00%> (ø)
...service/DynamicallyActivatableServiceObserver.java 50.00% <50.00%> (ø)
.../propertysources/http/HttpPropertySourceState.java 77.86% <100.00%> (+0.17%) ⬆️
...ot/core/service/DynamicallyActivatableService.java 83.72% <100.00%> (+0.79%) ⬆️
.../ocelot/core/metrics/system/GCMetricsRecorder.java 72.95% <0.00%> (+0.82%) ⬆️

@heiko-holz heiko-holz changed the title Feature 1495 Closes #1495 - improve the behavior when agent commands fail (or are not available) due to missing configuration - backend Oct 17, 2022
Copy link
Contributor

@heiko-holz heiko-holz 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 r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @danipaniii)

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.

[Feature] - improve the behavior when agent commands fail (or are not available) due to missing configuration
3 participants