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

Aws faas #3340

Closed
wants to merge 24 commits into from
Closed

Aws faas #3340

wants to merge 24 commits into from

Conversation

raphw
Copy link
Contributor

@raphw raphw commented Sep 29, 2023

Factor out AWS module to not depend on agent-core module, adding Faas and ServiceOrigin abstraction.


Edit:
Fixes #3016

@github-actions github-actions bot added agent-java community Issues and PRs created by the community triage labels Sep 29, 2023
@github-actions
Copy link

👋 @raphw Thanks a lot for your contribution!

It may take some time before we review a PR, so even if you don’t see activity for some time, it does not mean that we have forgotten about it.

Every once in a while we go through a process of prioritization, after which we are focussing on the tasks that were planned for the upcoming milestone. The prioritization status is typically reflected through the PR labels. It could be pending triage, a candidate for a future milestone, or have a target milestone set to it.

SylvainJuge
SylvainJuge previously approved these changes Oct 9, 2023
Copy link
Member

@SylvainJuge SylvainJuge left a comment

Choose a reason for hiding this comment

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

Another awesome refactor, thanks a lot @raphw !! I am approving this to get it running in CI.


import javax.annotation.Nullable;

public interface DirectSpan<T extends DirectSpan<T>> extends AbstractDirectSpan<T>, Span<T> {
Copy link
Member

Choose a reason for hiding this comment

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

[question] I am not sure to understand what is the purpose and benefit of this direct package and the classes within it, could you clarify a bit here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I stacked these PRs as they touch quite some of the same code and I wanted to avoid lengthy merges, I should have documented that order, sorry. The direct package can be considered as API for creating explicit spans and recording explicit data as with open tracing and open telementry. Maybe explicit would be a better name for it.

.describedAs("sample rate header should return same instance on each call")
.isSameAs(sampler.getTraceStateHeader());
}

Copy link
Member

Choose a reason for hiding this comment

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

[question] what makes this test not relevant anymore ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is still there, it is just moved to the tracer-api module. The header code is however moved out, so some of the code was removed.

public LifecycleListener getInitListener() {
return new AbstractLifecycleListener() {
public InitializableLifecycleListener getInitListener() {
class InitListener extends AbstractLifecycleListener implements InitializableLifecycleListener {
Copy link
Member

Choose a reason for hiding this comment

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

[minor] why not use a simple anonymous class here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An anonymous class cannot implement two classes/interfaces as the same time.

Map<String, Object> tags,
String operationName, long microseconds,
@Nullable Iterable<Map.Entry<String, String>> baggage, ClassLoader applicationClassLoader) {
AbstractSpan<?> result = null;
ElasticApmTracer tracer = OpenTracingBridgeInstrumentation.tracer.require(ElasticApmTracer.class);
Copy link
Member

Choose a reason for hiding this comment

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

[question] does it means that we get rid of the OpenTracing bridge ? We are thinking about removing that for a while, but doing so would also require us to officially deprecate and document this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably, but I would separate this out into another PR.

…ializableLifecycleListener.java

Co-authored-by: SylvainJuge <[email protected]>
raphw added 3 commits October 9, 2023 21:30
# Conflicts:
#	apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/AbstractSpan.java
#	apm-agent-plugins/apm-awslambda-plugin/src/main/java/co/elastic/apm/agent/awslambda/helper/S3TransactionHelper.java
@botelastic
Copy link

botelastic bot commented Jan 19, 2024

Hi! We just realized that we haven't looked into this issue in a while. We're sorry! We're labeling this issue as stalled to make it hit our filters and make sure we get back to it in as soon as possible. In the meantime, it'd be extremely helpful if you could take a look at it as well and confirm its relevance. A simple comment with a nice emoji will be enough :+1. Thank you for your contribution!

@botelastic botelastic bot added the stalled label Jan 19, 2024
@botelastic
Copy link

botelastic bot commented Feb 2, 2024

Hi! This issue has been stale for a while and we're going to close it as part of our cleanup procedure. We appreciate your contribution and would like to apologize if we have not been able to review it, due to the current heavy load of the team. Feel free to re-open this issue if you think it should stay open. Thank you for your contribution!

@botelastic botelastic bot closed this Feb 2, 2024
@jackshirazi jackshirazi reopened this Feb 2, 2024
@botelastic botelastic bot removed the stalled label Feb 2, 2024
@raphw
Copy link
Contributor Author

raphw commented Feb 6, 2024

Closing this in favor of #3516

@raphw raphw closed this Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-java community Issues and PRs created by the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove duplicate metricset serialization logic
3 participants