java-sdk icon indicating copy to clipboard operation
java-sdk copied to clipboard

Otel Configuration on Testcontainers

Open salaboy opened this issue 1 year ago • 10 comments

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:

  • [x] Code compiles correctly
  • [x] Created/updated tests
  • [ ] Extended the documentation

salaboy avatar Sep 13 '24 12:09 salaboy

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

salaboy avatar Sep 16 '24 11:09 salaboy

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

  • Add a marker interface named ConfigurationSpec - this would allow us to mark all the other configuration with this interface, so it is easily discoverable in an IDE.
  • Add a configuration spec builder - it is easier to follow what we try to put in a configuration spec if we use a builder approach. Having classes with constructors that have multiple parameters is not very user friendly. I would use something like the DaprContainer approach where you have with... which allows you to add different options.

Let me know your thoughts.

artur-ciocanu avatar Sep 24 '24 07:09 artur-ciocanu

@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 avatar Sep 24 '24 08:09 salaboy

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 avatar Sep 24 '24 08:09 salaboy

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:

  • TracingConfigurationSection
  • MetricsConfigurationSection
  • LoggingConfigurationSection
  • MiddlewareConfigurationSection
  • etc

Let me know your thoughts.

artur-ciocanu avatar Sep 24 '24 09:09 artur-ciocanu

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

artur-ciocanu avatar Sep 26 '24 13:09 artur-ciocanu

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

done

salaboy avatar Sep 27 '24 07:09 salaboy

@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 avatar Oct 03 '24 09:10 artur-ciocanu

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 avatar Oct 04 '24 09:10 salaboy

@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 avatar Oct 04 '24 12:10 salaboy

@artur-ciocanu I can close this one right?

salaboy avatar Oct 30 '24 09:10 salaboy

@artur-ciocanu I can close this one right?

I think so

artur-ciocanu avatar Oct 30 '24 12:10 artur-ciocanu

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

artur-ciocanu avatar Nov 05 '24 08:11 artur-ciocanu