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

Implement message queue #457

Merged
merged 34 commits into from
Dec 7, 2022

Conversation

secustor
Copy link
Member

@secustor secustor commented Oct 17, 2022

Fixes #429

Changes

  • add Kafka to docker-compose
  • add Kafka producer to checkoutservice
  • implement Accountingservice
  • implement Frauddetectionservice

For significant contributions please make sure you have completed the following items:

@cartersocha
Copy link
Contributor

Hey just an fyi we're on a feature freeze for the next week or 2 due to our upcoming v1 release and kubecon.

Feel free to keep this open and working on it tho!

src/accountingservice/README.md Outdated Show resolved Hide resolved
src/frauddetectionservice/README.md Outdated Show resolved Hide resolved
@secustor secustor marked this pull request as ready for review October 27, 2022 11:00
@secustor secustor requested a review from a team October 27, 2022 11:00
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@fatsheep9146
Copy link
Contributor

fatsheep9146 commented Oct 31, 2022

I wonder that why add two new services? Since the checkout service is the only kafka message producer, the two new services are both consumers. What's the point of this?

@secustor
Copy link
Member Author

Async workflows usually have multiple consumers with different use cases.
The intention is to show async with OTEL in a common scenario and that both spans are added to the parent span of the checkoutservice

docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
src/accountingservice/README.md Outdated Show resolved Hide resolved
@secustor secustor requested a review from mic-max November 2, 2022 19:29
Copy link
Contributor

@saurabhdes saurabhdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@cartersocha
Copy link
Contributor

we should test the build time increase from 3 new containers. One thing to keep in mind is that go doesn’t have an operator instrumentation option. We’d need to implement that with the Java service or replace Go

@cartersocha
Copy link
Contributor

For the architecture docs and services table my bias would be to label the language Java not Kotlin for the fraud detection service

@secustor
Copy link
Member Author

secustor commented Nov 26, 2022

I have tested builds again this branch and main.

Setup:

  • Mac M1
  • ContainerRuntime:
    • DockerDaemon: Colima
    • 8 GB RAM
    • 4 Cores
    • 100 GB Disk

Methodology
Run docker system prune --force && docker-compose build --no-cache

implemented changes:

  • 12m 11s
  • 13m 22s

main ( 5a2c0ef ):

  • 12m 30s
  • 12m 58s

So there are no relevant changes. The limiting factor is the build time of the currency service.

For the architecture docs and services table my bias would be to label the language Java not Kotlin for the fraud detection service

I prefer to use Kotlin as this signals that multiple JVM languages are supported. An alternative would be to rename JAVA to JVM

@secustor
Copy link
Member Author

secustor commented Dec 5, 2022

From the Google docs:

Merge PR then re-write new service?

@julianocosta89 @cartersocha @puckpuck I have not been able to join the SIG Meeting, is there a decision to rewrite accountingservice in Python before or after the merge of this PR?

@puckpuck
Copy link
Contributor

puckpuck commented Dec 5, 2022

The goal is to get this PR merged this week with Go for the accountingservice

After the merge, we will rewrite the service in another language that can be auto-instrumented with the OpenTelemetry Operator. We may want to document that as part of this PR so expectations are set with the broader community.

Copy link
Contributor

@cartersocha cartersocha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Ready to merge after @julianocosta89 or @puckpuck get a chance to take a look.

Thank you @secustor for the big addition!

Copy link
Contributor

@puckpuck puckpuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for this. Docs are great too. I do have some non-functional items that I would like addressed, but everything else looks ready to go.

docker-compose.yml Outdated Show resolved Hide resolved
src/accountingservice/kafka/consumer.go Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
Signed-off-by: Pierre Tessier <[email protected]>
Signed-off-by: Pierre Tessier <[email protected]>
Copy link
Contributor

@puckpuck puckpuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made some changes to logging as well as added health checks.

I also changed the name of the kafka container to: kafka

@cartersocha cartersocha merged commit 780172d into open-telemetry:main Dec 7, 2022
@puckpuck
Copy link
Contributor

puckpuck commented Dec 7, 2022

🎉

@secustor secustor deleted the implement_message_queue branch December 7, 2022 08:13
jmichalak9 pushed a commit to jmichalak9/opentelemetry-demo that referenced this pull request Mar 22, 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.

feat: Add queue example to demo
9 participants