search

dapr / java-sdk

Dapr SDK for Java
Apache License 2.0
262 stars 207 forks source link

Otel Configuration on Testcontainers #1126

Open salaboy opened 2 months ago

salaboy commented 2 months ago

Description

Implement Configuration for Testcontainers

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #1125

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

salaboy commented 2 months ago

@artursouza @cicoyle please review. This is the first step to demonstrate OTEL with local development.

artur-ciocanu commented 2 months ago

@salaboy I have checked the changes and I have a few general suggestions:

Let me know your thoughts.

salaboy commented 2 months ago

@artur-ciocanu I agree, but let's do this in two separate steps.. the Configuration class first and then we can add the fluent buileres to it. I am building an example where I want to test if this configurations work or not and based on that I want to make sure that the experience is polished with builders and all.

salaboy commented 2 months ago

Regarding the ConfigurationSpec interface.. i don't think it makes sense.. if we just have a single configuration CRD that covers a bunch of different topics -> https://docs.dapr.io/operations/configuration/configuration-overview/ Maybe I am missing something

artur-ciocanu commented 2 months ago

Regarding the ConfigurationSpec interface.. i don't think it makes sense.. if we just have a single configuration CRD that covers a bunch of different topics -> https://docs.dapr.io/operations/configuration/configuration-overview/ Maybe I am missing something

@salaboy thanks for feedback, my proposal was to have an interface that would allow us to model different configuration sections. It can be an interface named ConfigurationSpec or ConfigurationSection or ConfigurationParameters. Then we can have different implementations for different sections like:

Let me know your thoughts.

artur-ciocanu commented 2 months ago

@salaboy if it is not too much to ask, could you please rebase your branch. It is outdated.

salaboy commented 1 month ago

@salaboy if it is not too much to ask, could you please rebase your branch. It is outdated.

done

artur-ciocanu commented 1 month ago

@salaboy I was looking at the latest changes you pushed, the ones related to Observation API. In my opinion it would be easier to review if we have the following:

Let me know your thoughts.

salaboy commented 1 month ago

I made good progress here.. now traces are propagated in some way for messaging, we need to do the same for the kvstore + bindings but the messaging side of things should show us the way forward. If you look at this line : https://github.com/dapr/java-sdk/pull/1126/files#diff-f86931381933e4ae24b3f860afc7618f23aecb516cea8bfd0ce627b16bf64e19R159 this is propagating the trace parent to the grpc call @jonatan-ivanov @onobc but the sequencing might be wrong, as the spans are parallel and not one inside the other as shown in this screenshot:

Screenshot 2024-10-03 at 21 31 43

Here is the node graph:

Screenshot 2024-10-04 at 10 46 57

In theory, we should see "Rest Endpoint -> Messaging template bean -> GRPC call to the Dapr Sidecar", but at least now the traceparent is propagated and all the spans belong to the same traceparent.

salaboy commented 1 month ago

@salaboy I was looking at the latest changes you pushed, the ones related to Observation API. In my opinion it would be easier to review if we have the following:

  • One PR for OTEL configs for Dapr Testcontainer
  • Second PR for Micrometer Observation API support for new Dapr Spring Boot integration

Let me know your thoughts.

@artur-ciocanu I will appreciate any help that you can provide here.. I am happy for you to copy the contents of this PR in separate PRs if you think that is best, I will be away until the 13th of October so any help on moving this forward will be highly appreciated.

salaboy commented 3 weeks ago

@artur-ciocanu I can close this one right?

artur-ciocanu commented 3 weeks ago

@artur-ciocanu I can close this one right?

I think so

artur-ciocanu commented 2 weeks ago

@salaboy could you please close this PR and we could focus on other observation enhancements.