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

JS-498 Add runtime telemetry #5017

Merged
merged 6 commits into from
Dec 18, 2024
Merged

JS-498 Add runtime telemetry #5017

merged 6 commits into from
Dec 18, 2024

Conversation

zglicz
Copy link
Contributor

@zglicz zglicz commented Dec 16, 2024

JS-498

Adding runtime telemetry as per the bonus part of the MMF.
Refactoring the code a bit, so only querying the node server, if telemetry is actually enabled.

@zglicz zglicz requested a review from a team December 16, 2024 10:25
@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod bot changed the title Add runtime telemetry JS-498 Add runtime telemetry Dec 16, 2024
Copy link
Contributor

@saberduck saberduck left a comment

Choose a reason for hiding this comment

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

I think it would be better not to expose NodeCommand outside of the bridge module. We could instead just add more fields to the Telemetry record

@@ -111,8 +111,7 @@ void analyzeFiles(List<InputFile> inputFiles) throws IOException {
)
);
}
var telemetry = bridgeServer.getTelemetry();
new PluginTelemetry(context).reportTelemetry(telemetry);
new PluginTelemetry(context, bridgeServer).reportTelemetry();
Copy link
Contributor

Choose a reason for hiding this comment

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

optional - I would only pass value from bridgeServer.getTelemtry() instead of passing the whole bridgeServer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way, if telemetry is not available or not necessary to compute for some reason, we wouldn't be computing it.

@zglicz zglicz merged commit b0f6f52 into master Dec 18, 2024
20 of 21 checks passed
@zglicz zglicz deleted the runtime-telemetry branch December 18, 2024 10:45
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.

2 participants