Skip to content

Conversation

@salaboy
Copy link
Collaborator

@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 salaboy requested review from a team as code owners September 13, 2024 12:20
@salaboy
Copy link
Collaborator 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
Collaborator 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
Collaborator 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
Copy link
Collaborator 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
Collaborator 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.

artur-ciocanu and others added 14 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]>
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
Collaborator 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
Collaborator 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.

@salaboy salaboy closed this Dec 3, 2024
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