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

Async resource providers #31

Merged
merged 14 commits into from
Nov 10, 2023

Conversation

SylvainJuge
Copy link
Member

@SylvainJuge SylvainJuge commented Nov 9, 2023

  • fix mockserver container port check
  • first impl. of async resource
  • make resource loading async

Impact on agent init evaluated with AWS resource provider test: AwsResourceProvidersTest

  • with async loading: about 51.5 seconds
  • without async loading: about 60 seconds
    Test execution duration covers multiple providers and is dominated by container startup time, thus it means about 2-3 seconds added to the application startup if resource execution is synchronous.

Map<String, String> extraConfig = new HashMap<>();
extraConfig.put(
"otel.java.disabled.resource.providers",
ElasticResourceProvider.class.getCanonicalName());
Copy link
Member Author

Choose a reason for hiding this comment

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

[for reviewer] This might be a good alternative to the rather complex re-packaging of the contrib resource providers to prevent them from being executed during SDK startup, but we can do that in a follow-up improvement.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this overwrite or append the option?

import io.opentelemetry.sdk.metrics.data.MetricDataType;
import io.opentelemetry.sdk.resources.Resource;

public class DelegatingMetricData implements MetricData {
Copy link
Member Author

Choose a reason for hiding this comment

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

[for reviewer] there is no equivalent for metrics to io.opentelemetry.sdk.trace.data.DelegatingSpanData that we already use for exporting spans, thus just fills the void and could be a relevant contribution to the upstream project.

import java.util.concurrent.TimeUnit;
import java.util.logging.Logger;

public class ExecutorUtils {
Copy link
Member Author

Choose a reason for hiding this comment

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

[for reviewer] mostly copied from our current agent codebase.

Comment on lines 51 to 55
this.config = config;
if(!withExtra){
return getBaseResource(config);
}
return getBaseResource(config).merge(getExtraResource());
Copy link
Member Author

Choose a reason for hiding this comment

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

[for reviewer] this allows to use this provider with the SDK and make it fetch the extra attributes, or use it asynchronously with our agent distribution to limit application startup overhead.

@SylvainJuge SylvainJuge self-assigned this Nov 9, 2023
@SylvainJuge SylvainJuge requested a review from a team November 9, 2023 10:46
@@ -62,6 +62,12 @@ public static boolean isDebugging() {
return false;
}

public static boolean isDebugInCI() {
Copy link
Member Author

Choose a reason for hiding this comment

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

[for reviewer] this is especially relevant to help debugging tests failing in CI (there is an extra checkbock when trying to re-run them).

@SylvainJuge SylvainJuge marked this pull request as ready for review November 9, 2023 14:35
@SylvainJuge SylvainJuge merged commit 2a62390 into elastic:main Nov 10, 2023
2 checks passed
@SylvainJuge SylvainJuge deleted the async-resource-providers branch November 10, 2023 13:16
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