-
Notifications
You must be signed in to change notification settings - Fork 0
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
implementation to get dynamic config during runtime #7
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice start, could we add a test to verify the behaviour? You can spin up a a mocked backed service and then get the config from it.
configBuilder.setEnabled(response.getEnabled()); | ||
configBuilder.setDataCapture(response.getDataCapture()); | ||
configBuilder.setJavaagent(response.getJavaAgent()); | ||
agentConfig = configBuilder.build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the field from the HypertraceConfig class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we worry about concurrent modifications (read/write) at the same time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the field from HypertraceConfig class. We do not need to worry about concurrent modifications as it is a volatile field.
|
||
private final ConfigurationServiceGrpc.ConfigurationServiceBlockingStub blockingStub; | ||
|
||
private static Int64Value configTimestamp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
give it a better name.
DynamicConfigFetcher dynamicConfigFetcher = null; | ||
if (dynamicConfigServiceUrl != null) { | ||
dynamicConfigFetcher = new DynamicConfigFetcher(dynamicConfigServiceUrl); | ||
Executors.newScheduledThreadPool(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an executor service in the upstream that we could use? The upstream javaagent-tooling might have smth that we could reuse. We also have to set the thread to the daemon otherwise it will be blocking shutdown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will check this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upstream does not have any executor service that I could find. They have also used Executors directly https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/SupportabilityMetrics.java#L81
Assuming grpc service will be similar to what is in hypertrace/agent-config#61.