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.

Sign up for GitHub

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

Jump to bottom

Add Apolloconfig Inst #12794

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add Apolloconfig Inst #12794

wants to merge 3 commits into from

Conversation

freshchen
Copy link

#12787

First commit, please help review

@freshchen freshchen requested a review from a team as a code owner November 26, 2024 04:12
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Nov 26, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@freshchen freshchen force-pushed the dev_apollo_config branch 3 times, most recently from 8e921aa to e6ad0ff Compare November 26, 2024 06:41
laurit
laurit reviewed Nov 26, 2024
View reviewed changes
...try/javaagent/instrumentation/apolloconfig/v2_0_0/ApolloRepositoryChangeInstrumentation.java Outdated
return;
}

context = instrumenter().start(parentContext, namespace);
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps instead of spans this should be modeled as events?

Copy link
Author

Choose a reason for hiding this comment

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

The logic of this library is roughly as follows

  • watch ( http long pull. etc. )
  • notify (Our current advise point)
    • listener1：spring env update
    • listener2：spring event
    • listener3：user custom，may have call
    • ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Besides creating a span we could consider an event like in

opentelemetry-java-instrumentation/instrumentation/runtime-telemetry/runtime-telemetry-java8/javaagent/src/main/java/io/opentelemetry/instrumentation/javaagent/runtimemetrics/java8/JarAnalyzer.java

Line 242 in 3fd27aa

eventLogger.builder(EVENT_NAME_INFO).setAttributes(builder.build()).emit();
@trask WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

an event sounds like a good option here

Copy link
Author

Choose a reason for hiding this comment

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

I am not very familiar with event signals, which look like logs. I am not sure whether to remove span and replace it with event or add an event based on span.

If it is just one line of event log, I don't think it is helpful. The detailed background is as follows #12787 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

it looks like this has been added to today's Java APAC-friendly meeting, let's chat then

steverao
steverao reviewed Nov 26, 2024
View reviewed changes
Copy link
Contributor

@steverao steverao left a comment

Choose a reason for hiding this comment

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

Don't forget to add the framework to document.

@freshchen freshchen force-pushed the dev_apollo_config branch 4 times, most recently from 031bb5f to f657fc4 Compare November 27, 2024 06:45
@freshchen
Copy link
Author

Don't forget to add the framework to document.

@steverao Hao Zong，the document link is wrong

@freshchen freshchen force-pushed the dev_apollo_config branch 3 times, most recently from 392c710 to 8b836ae Compare November 27, 2024 09:45
@steverao
Copy link
Contributor

the document link is wrong

Sorry, this is related hyperlink, you can check it out.

@github-actions github-actions bot requested a review from theletterf November 27, 2024 11:09
steverao
steverao reviewed Nov 27, 2024
View reviewed changes
settings.gradle.kts Outdated Show resolved Hide resolved
@freshchen freshchen mentioned this pull request Nov 28, 2024
Closed
laurit
laurit reviewed Dec 3, 2024
View reviewed changes
docs/supported-libraries.md Outdated Show resolved Hide resolved
laurit
laurit reviewed Dec 3, 2024
View reviewed changes
...aagent/instrumentation/apolloconfig/apolloclient/v1_0/ApolloConfigInstrumentationModule.java
@AutoService(InstrumentationModule.class)
public class ApolloConfigInstrumentationModule extends InstrumentationModule {
public ApolloConfigInstrumentationModule() {
super("apolloconfig-apolloclient", "apolloconfig-apolloclient-1.0");
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe apolloconfig-client? I guess the package name and module name could also be adjusted accordingly.

Copy link
Author

Choose a reason for hiding this comment

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

It seems that we don't have a unified standard

image

@freshchen freshchen force-pushed the dev_apollo_config branch 7 times, most recently from de53eb9 to 310cb41 Compare December 6, 2024 05:52
laurit
laurit reviewed Dec 6, 2024
View reviewed changes
...nt/instrumentation/apolloconfig/apolloclient/v1_0/ApolloRepositoryChangeInstrumentation.java
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope) {
Context parentContext = currentContext();
String repeat = parentContext.get(REPOSITORY_CHANGE_REPEAT_CONTEXT_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

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

usually we use CallDepth to instrument only the outer most call, see

opentelemetry-java-instrumentation/instrumentation/jaxrs/jaxrs-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jaxrs/v1_0/JaxrsAnnotationsInstrumentation.java

Line 78 in 57c7cf2

callDepth = CallDepth.forClass(Path.class);

laurit
laurit reviewed Dec 6, 2024
View reviewed changes
instrumentation/apolloconfig-apolloclient-1.0/javaagent/build.gradle.kts
library("com.ctrip.framework.apollo:apollo-client:1.0.0")

testImplementation("com.ctrip.framework.apollo:apollo-client:1.1.0")
testImplementation(project(":testing-common"))
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need to add explicit dependency to testing-common, it is already added elsewhere

laurit
laurit reviewed Dec 6, 2024
View reviewed changes
instrumentation/apolloconfig-apolloclient-1.0/javaagent/build.gradle.kts

library("com.ctrip.framework.apollo:apollo-client:1.0.0")

testImplementation("com.ctrip.framework.apollo:apollo-client:1.1.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use testCompileOnly("com.ctrip.framework.apollo:apollo-client:1.1.0") that way tests would run with 1.0.0. Then you will have to make TestConfigRepository and TestRepositoryChangeListener private to avoid junit looking for test methods there.

laurit
laurit reviewed Dec 6, 2024
View reviewed changes
...lemetry/javaagent/instrumentation/apolloconfig/apolloclient/v1_0/ApolloConfigSingletons.java
@Nullable Throwable error) {}
};

INSTRUMENTER =
Copy link
Contributor

Choose a reason for hiding this comment

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

This was discussed at the sig meeting yesterday and the consensus was that events might be more appropriate for this use case. What do you think about creating an event instead of a span? If you plan to watch the recording of the sig meeting then this was discussed at the end of the meeting.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't quite understand the definition of events before. Is it this https://opentelemetry.io/docs/specs/otel/logs/event-api/

Is there a link about the meeting?

Copy link
Contributor

Choose a reason for hiding this comment

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

See

opentelemetry-java-instrumentation/instrumentation/runtime-telemetry/runtime-telemetry-java8/javaagent/src/main/java/io/opentelemetry/instrumentation/javaagent/runtimemetrics/java8/JarAnalyzer.java

Line 242 in 57c7cf2

eventLogger.builder(EVENT_NAME_INFO).setAttributes(builder.build()).emit();
for an existing event, it also has a test. You can find the meeting recoding in https://docs.google.com/spreadsheets/d/1SYKfjYhZdm2Wh2Cl6KVQalKg_m4NhTPZqq-8SzEVO6s/edit?pli=1&gid=0#gid=0

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your careful review. I took a quick look at the meeting and found some deviations. It is true that this framework is mainly popular in China. I will add some detailed explanations in the issue tomorrow.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, I added some information, hope it helps. #12787 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlchemyDing AlchemyDing AlchemyDing left review comments

@steverao steverao steverao left review comments

@laurit laurit laurit left review comments

@trask trask trask left review comments

@theletterf theletterf Awaiting requested review from theletterf

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@freshchen @steverao @trask @laurit @AlchemyDing