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

Otel Configuration on Testcontainers #1126

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

salaboy
Copy link
Contributor

@salaboy salaboy commented Sep 13, 2024

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:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@salaboy
Copy link
Contributor Author

salaboy commented Sep 16, 2024

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

@artur-ciocanu
Copy link
Contributor

@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.

@salaboy
Copy link
Contributor Author

salaboy commented Sep 24, 2024

@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
Copy link
Contributor Author

salaboy commented Sep 24, 2024

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
Copy link
Contributor

artur-ciocanu commented Sep 24, 2024

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
Copy link
Contributor

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

@salaboy salaboy force-pushed the testcontainers-conf branch 2 times, most recently from a1ac64e to 06ff43b Compare September 27, 2024 07:24
@salaboy
Copy link
Contributor Author

salaboy commented Sep 27, 2024

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

done

@artur-ciocanu
Copy link
Contributor

artur-ciocanu commented Oct 3, 2024

@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.

@salaboy
Copy link
Contributor Author

salaboy commented Oct 4, 2024

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 salaboy force-pushed the testcontainers-conf branch 3 times, most recently from 7fefd1b to a849dc5 Compare October 4, 2024 10:14
artur-ciocanu and others added 10 commits October 4, 2024 11:15
* Adding Maven Profiles

Signed-off-by: Artur Ciocanu <[email protected]>

* Simplify profiles setup

Signed-off-by: Artur Ciocanu <[email protected]>

---------

Signed-off-by: Artur Ciocanu <[email protected]>
Co-authored-by: Artur Ciocanu <[email protected]>
Signed-off-by: salaboy <[email protected]>
Update binding http IT

Signed-off-by: Artur Souza <[email protected]>
Signed-off-by: salaboy <[email protected]>
Signed-off-by: salaboy <[email protected]>
Signed-off-by: salaboy <[email protected]>
* Ensure we use the same GRPC version everywhere

Signed-off-by: Artur Ciocanu <[email protected]>

* Fix actors tests assert

Signed-off-by: Artur Ciocanu <[email protected]>

* Revert Dapr exception asserts

Signed-off-by: Artur Ciocanu <[email protected]>

* Increase sleep to allow Spring Context to bootstrap

Signed-off-by: Artur Ciocanu <[email protected]>

* Revert sleep value

Signed-off-by: Artur Ciocanu <[email protected]>

* Increase the sleep for messaging test

Signed-off-by: Artur Ciocanu <[email protected]>

* Move sleep before each, to ensure Spring context starts

Signed-off-by: Artur Ciocanu <[email protected]>

* Add more delays to ensure Spring Controller gets the messages

Signed-off-by: Artur Ciocanu <[email protected]>

---------

Signed-off-by: Artur Ciocanu <[email protected]>
Co-authored-by: Artur Ciocanu <[email protected]>
Signed-off-by: salaboy <[email protected]>
* Remove all global state in from setProperty

Signed-off-by: Artur Souza <[email protected]>

* use Map.of

Signed-off-by: Artur Souza <[email protected]>

* Remove dependency that is not needed.

Signed-off-by: Artur Souza <[email protected]>

---------

Signed-off-by: Artur Souza <[email protected]>
Signed-off-by: salaboy <[email protected]>
Signed-off-by: salaboy <[email protected]>
artursouza and others added 3 commits October 4, 2024 11:15
Update binding http IT

Signed-off-by: Artur Souza <[email protected]>
Signed-off-by: salaboy <[email protected]>
* Remove all global state in from setProperty

Signed-off-by: Artur Souza <[email protected]>

* use Map.of

Signed-off-by: Artur Souza <[email protected]>

* Remove dependency that is not needed.

Signed-off-by: Artur Souza <[email protected]>

---------

Signed-off-by: Artur Souza <[email protected]>
Signed-off-by: salaboy <[email protected]>
Signed-off-by: salaboy <[email protected]>
@salaboy
Copy link
Contributor Author

salaboy commented Oct 4, 2024

@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
Copy link
Contributor Author

salaboy commented Oct 30, 2024

@artur-ciocanu I can close this one right?

@artur-ciocanu
Copy link
Contributor

@artur-ciocanu I can close this one right?

I think so

@artur-ciocanu
Copy link
Contributor

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

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.

Support for Dapr Configuration on DaprContainer (testcontainers)
4 participants