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

chore: init metrics only if monitoring enabled #5165

Merged
merged 6 commits into from
Nov 7, 2023

Conversation

jdrueckert
Copy link
Member

@jdrueckert jdrueckert commented Nov 7, 2023

When looking into the integration test output, I noticed monitoring-related errors and thought that we don't need monitoring when running integration tests in the first place. On looking into the monitoring subsystem, I noticed that we do not create the AdvancedMonitor if monitoring is not enabled, but we still initialized metrics etc. I don't think this is necessary if monitoring is not enabled, so this PR changes that.

Also, the sporadically failing tests typically time out on creating a client (in particular, joining a client to the host). I added a new integration test that simply tests that client creation does not time out. Unfortunately, I was not able to reproduce the timeout situation locally. Still I think that this test might be helpful.

Finally, I noticed that in the example test testSendEvent we're sending a ResetCameraEvent which I thought to be weirdly specific and potentially introducing issues. If the goal of a test is to only test that sending an event doesn't fail, we should use a minimal dummy event and test specific events in a dedicated fashion. This is why, I changed the test to send DummyEvent instead.

TL;DR This PR contains

  • initializing metrics only if monitoring is enabled
  • adding client creation test to ensure clients can be created and connect to hosts without experiencing a timeout
  • sending DummyEvent instead of ResetCameraEvent to avoid unintended side effects and delays that might falsify the intended test result

I'm not at all sure or convinced that these changes fix the sporadics, however they might add to making them more reliable.

@github-actions github-actions bot added the Type: Chore Request for or implementation of maintenance changes label Nov 7, 2023
@jdrueckert jdrueckert marked this pull request as ready for review November 7, 2023 12:28
@jdrueckert jdrueckert added Type: Improvement Request for or addition/enhancement of a feature Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness Size: S Small effort likely only affecting a single area and requiring little to no research labels Nov 7, 2023
@jdrueckert jdrueckert added this to the 2023 Revive - Milestone 2 milestone Nov 7, 2023
@jdrueckert jdrueckert merged commit 0342fad into develop Nov 7, 2023
9 checks passed
@jdrueckert jdrueckert deleted the chore/metrics-only-if-monitoring branch November 7, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: S Small effort likely only affecting a single area and requiring little to no research Topic: Stabilization Requests, Issues and Changes related to improving stablity and reducing flakyness Type: Chore Request for or implementation of maintenance changes Type: Improvement Request for or addition/enhancement of a feature
Projects
Status: No status
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants