store order to postgresql from accounting service #2175
Conversation
github-actions
bot
added
the
helm-update-required
|
https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Instrumentation.EntityFrameworkCore used to instrument this package is not yet on stable semantic convention. It is using the old one. Happy to see contributions to move it forward! : ) |
docker-compose.yml
Outdated
| @@ -829,3 +830,15 @@ services: | |||
| timeout: 10s | |||
| retries: 10 | |||
| logging: *logging | |||
|
|
|||
| postgresql: | |||
There was a problem hiding this comment.
Please move this entire section to the "Dependent Services" section of this file. It should be in that section alphabetically based on the service name, so around line 681.
docker-compose.yml
Outdated
| @@ -829,3 +830,15 @@ services: | |||
| timeout: 10s | |||
| retries: 10 | |||
| logging: *logging | |||
|
|
|||
| postgresql: | |||
| image: 'postgres:latest' | |||
There was a problem hiding this comment.
This needs to be based on an environment variable called POSTGRES_IMAGE, set up similarly to how we do this for Valkey.
src/accounting/init.sql
Outdated
There was a problem hiding this comment.
This file should be in a src/postgres/ folder instead. We would also need to update docker-compose to reference the new location.
…nto add-postgresql-for-accounting
…opentelemetry-demo into add-postgresql-for-accounting
|
@puckpuck all the above comments have been resolved. please kindly take a look. thanks! |
src/accounting/Consumer.cs
Outdated
| @@ -38,6 +58,7 @@ public void StartListening() | |||
| { | |||
| try | |||
| { | |||
| using var activity = MyActivitySource.StartActivity("order-consumed"); | |||
There was a problem hiding this comment.
somehow the db activity is not automatically closing. so i added this to group the child spans. otherwise, it would result in recursively nested spans like this
There was a problem hiding this comment.
@Kielek do you have any insights here?
There was a problem hiding this comment.
also have raised in the dotnet repo open-telemetry/opentelemetry-dotnet-contrib#2792
There was a problem hiding this comment.
At a first glance, it seems to be connected to interaction of 2 instrumentations active at the same time (EFCore and Npsql).
As a quick workaround, you can try disabling one of them by e.g. setting the following in docker-compose file:
OTEL_DOTNET_AUTO_TRACES_ENTITYFRAMEWORKCORE_INSTRUMENTATION_ENABLED=false
See the docs for a more info how to disable specific instrumentation.
Changes from e191c3e should be reverted. If custom instrumentation is needed, this describes the recommended approach.
There was a problem hiding this comment.
thanks for pointing out! did not notice npsql also have native instrumentation. have updated accordingly and it is working as expected now
This reverts commit e191c3e.
Changes
Add postgresql database, for accounting service to store orders receive from kafka. The current change only reflected in docker-compose and not in the kubernetes deployment yet. It will fallback to not use database if there is no corresponding environment variable. So I think it wont break the kubernetes deployment. But if we think it is necessary to have the kubernetes change in this PR as well, I will update later.
Fixes #912
Merge Requirements
For new features contributions, please make sure you have completed the following
essential items:
CHANGELOG.mdupdated to document new feature additionsMaintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.