blog(add article): how to contribute to the otel demo #3671
Conversation
There was a problem hiding this comment.
Thank you for suggesting this blog post, @adnanrahic. I appreciate the effort that you took to write it down, however there are some major concerns I have with the current form, I outlined them in my review comments already, but to summarize:
- The blog post suggest that it teaches you how to contribute to the otel demo, I don't see anything in the blog post around creating a PR, committing it, interacting with the community etc.
- The blog post makes unnecessary use of tracetest. Since tracetest is oss, there are cases where it can be used (like in https://opentelemetry.io/blog/2023/testing-otel-demo/), but it doesn't fit the title and content of the blog post. Please make use of Jaeger.
- The blog post is very long, so I made a lot of suggestions around reducing the size, mainly to skip a major part at the beginning explaining the demo, and that you start with a fork without the instrumentation and work from there.
|
|
||
| [ Output ] | ||
| docker compose run traceBasedTests payment-service | ||
| [+] Creating 21/0 | ||
| ✔ Container jaeger Running 0.0s | ||
| ✔ Container kafka Running 0.0s | ||
| ✔ Container tracetest-postgres Running 0.0s | ||
| ✔ Container postgres Running 0.0s | ||
| ✔ Container redis-cart Running 0.0s | ||
| ✔ Container feature-flag-service Running 0.0s | ||
| ✔ Container otel-col Running 0.0s | ||
| ✔ Container currency-service Running 0.0s | ||
| ✔ Container cart-service Running 0.0s | ||
| ✔ Container payment-service Running 0.0s | ||
| ✔ Container tracetest-server Running 0.0s | ||
| ✔ Container quote-service Running 0.0s | ||
| ✔ Container accounting-service Running 0.0s | ||
| ✔ Container frauddetection-service Running 0.0s | ||
| ✔ Container product-catalog-service Running 0.0s | ||
| ✔ Container ad-service Running 0.0s | ||
| ✔ Container email-service Running 0.0s | ||
| ✔ Container recommendation-service Running 0.0s | ||
| ✔ Container shipping-service Running 0.0s | ||
| ✔ Container checkout-service Running 0.0s | ||
| ✔ Container frontend Running 0.0s | ||
| [+] Running 3/3 | ||
| ✔ Container postgres Healthy 0.5s | ||
| ✔ Container kafka Healthy 0.5s | ||
| ✔ Container tracetest-postgres Healthy 0.5s | ||
| Starting tests... | ||
|
|
||
| Running trace-based tests... | ||
|
|
||
| ✔ Payment Service (http://tracetest-server:11633/testsuite/payment-service-all/run/2) | ||
| ✔ Payment: valid credit card (http://tracetest-server:11633/test/payment-valid-credit-card/run/2/test) - trace id: 7774d4754957e5fa5b916e4d6d5880e7 | ||
| ✔ It should call Charge method successfully | ||
| ✔ It should return a transaction ID | ||
| ✔ It should return a valid credit card | ||
| ✔ Payment: invalid credit card (http://tracetest-server:11633/test/payment-invalid-credit-card/run/2/test) - trace id: a20c1d166cd6edf4d8f288e407e76623 | ||
| ✔ It should call Charge method and receive a gRPC error | ||
| ✔ It should return a return an gRPC error code to the caller | ||
| ✔ Payment: Amex credit card not allowed (http://tracetest-server:11633/test/payment-amex-credit-card-not-allowed/run/2/test) - trace id: a5812bf50ac61a387dc991ba0dd3020a | ||
| ✔ It should call Charge method and receive a gRPC error | ||
| ✔ It should return a return an gRPC error code to the caller | ||
| ✔ Payment: expired credit card (http://tracetest-server:11633/test/payment-expired-credit-card/run/2/test) - trace id: fecb6368104b0fc27e7806136fc1ab1c | ||
| ✔ It should call Charge method and receive a gRPC error | ||
| ✔ It should return a return an gRPC error code to the caller | ||
|
|
||
| Tests done! Exit code: 0 | ||
| ``` | ||
|
|
||
| You can also start the Tracetest services alongside all the Core Demo Services, | ||
| Dependent Services, and Telemetry Components as part of your development | ||
| lifecycle to enable | ||
| [Observability-driven Development (ODD)](https://stackoverflow.blog/2022/10/12/how-observability-driven-development-creates-elite-performers/). | ||
| Mainly to trigger your APIs and validate both the response and trace data they | ||
| generate. You can also build tests visually and save them as YAML files to add | ||
| to your code repository for automated testing. I’ll walk you through all of this | ||
| a bit later as well. | ||
|
|
||
| To do this, you’ll use the `odd` | ||
| [Docker Compose profile](https://docs.docker.com/compose/profiles/). Run the | ||
| demo like this: | ||
|
|
||
| ```bash | ||
| docker compose --profile odd up --force-recreate --remove-orphans --detach | ||
|
|
||
| # OR | ||
|
|
||
| make start-odd | ||
| ``` | ||
|
|
||
| Go ahead and start the OpenTelemetry Demo including Tracetest, with the `odd` | ||
| profile. | ||
|
|
||
| Once the images are built and containers are started you can access: | ||
|
|
||
| - Web store: [http://localhost:8080/](http://localhost:8080/) | ||
| - Grafana: [http://localhost:8080/grafana/](http://localhost:8080/grafana/) | ||
| - Feature Flags | ||
| UI: [http://localhost:8080/feature/](http://localhost:8080/feature/) | ||
| - Load Generator | ||
| UI: [http://localhost:8080/loadgen/](http://localhost:8080/loadgen/) | ||
| - Jaeger | ||
| UI: [http://localhost:8080/jaeger/ui/](http://localhost:8080/jaeger/ui/) | ||
| - Tracetest UI: [http://localhost:11633/](http://localhost:11633/), only when | ||
| using `make start-odd` | ||
|
|
||
| To run a test against the Payment Service, I’ll use a YAML file and trigger it | ||
| with the | ||
| [Tracetest CLI](https://docs.tracetest.io/getting-started/installation#install-the-tracetest-cli). | ||
| Alternatively, you can also build tests visually in the Tracetest UI | ||
| on [http://localhost:11633/](http://localhost:11633/). | ||
|
|
||
| > _Here’s a | ||
| > [short guide on how to create tests programatically](https://docs.tracetest.io/getting-started/open#creating-trace-based-tests-programatically) | ||
| > with Tracetest._ | ||
|
|
||
| Next, let’s move on to explaining and adding code instrumentation in the | ||
| `paymentservice`. |
There was a problem hiding this comment.
Can all of this be replaced with a reference to https://opentelemetry.io/docs/demo/ ? It duplicates a lot of words from there
| I’ve prepared a fork with detailed code examples and three demos. This will help | ||
| you understand how to add OpenTelemetry code instrumentation. | ||
|
|
||
| ```bash | ||
| git clone https://github.com/kubeshop/opentelemetry-demo.git | ||
| cd opentelemetry-demo/ | ||
| make start-odd | ||
| ``` |
There was a problem hiding this comment.
how is this fork different from upstream, when I follow that link I get This branch is up to date with open-telemetry/opentelemetry-demo:main., also I would prefer you to use your personal github and not your org github for such a fork
| ```jsx | ||
| // src/paymentservice/opentelemetry.js | ||
|
|
||
| const opentelemetry = require('@opentelemetry/sdk-node'); | ||
| const { | ||
| getNodeAutoInstrumentations, | ||
| } = require('@opentelemetry/auto-instrumentations-node'); | ||
| const { | ||
| OTLPTraceExporter, | ||
| } = require('@opentelemetry/exporter-trace-otlp-grpc'); | ||
| const { | ||
| OTLPMetricExporter, | ||
| } = require('@opentelemetry/exporter-metrics-otlp-grpc'); | ||
| const { PeriodicExportingMetricReader } = require('@opentelemetry/sdk-metrics'); | ||
| const { | ||
| alibabaCloudEcsDetector, | ||
| } = require('@opentelemetry/resource-detector-alibaba-cloud'); | ||
| const { | ||
| awsEc2Detector, | ||
| awsEksDetector, | ||
| } = require('@opentelemetry/resource-detector-aws'); | ||
| const { | ||
| containerDetector, | ||
| } = require('@opentelemetry/resource-detector-container'); | ||
| const { gcpDetector } = require('@opentelemetry/resource-detector-gcp'); | ||
| const { | ||
| envDetector, | ||
| hostDetector, | ||
| osDetector, | ||
| processDetector, | ||
| } = require('@opentelemetry/resources'); | ||
|
|
||
| const sdk = new opentelemetry.NodeSDK({ | ||
| // OTLPTraceExporter() uses the env var "OTEL_EXPORTER_OTLP_ENDPOINT" when not explicitly set. | ||
| traceExporter: new OTLPTraceExporter(), | ||
| instrumentations: [getNodeAutoInstrumentations()], | ||
| metricReader: new PeriodicExportingMetricReader({ | ||
| exporter: new OTLPMetricExporter(), | ||
| }), | ||
| resourceDetectors: [ | ||
| containerDetector, | ||
| envDetector, | ||
| hostDetector, | ||
| osDetector, | ||
| processDetector, | ||
| alibabaCloudEcsDetector, | ||
| awsEksDetector, | ||
| awsEc2Detector, | ||
| gcpDetector, | ||
| ], | ||
| }); | ||
| sdk.start(); | ||
| ``` |
There was a problem hiding this comment.
Can you make this (and other) code examples a little bit compacter? I think you can write more things in one line than you do it in the original code
| I’ll walk you through these three demos, with one sample without telemetry. | ||
|
|
||
| 1. **Demo 0**: What’s it like without OpenTelemetry traces? | ||
| 2. **Demo 1**: Get the active span from the context and use it as the main span | ||
| in the `chargeServiceHandler` . | ||
| 3. **Demo 2**: Get the active span from the context to create a new context. | ||
| Create a new span for the `chargeServiceHandler` and pass the new context in | ||
| as a parameter. | ||
| 4. **Demo 3**: Create a new active span for the `chargeServiceHandler` without | ||
| the need to pass a parent span and context. | ||
|
|
||
| But first, let’s make it harder! It’s easy to get started when somebody is | ||
| holding your hand. Let’s remove the guardrails for a second. Literally, let’s | ||
| remove the OpenTelemetry instrumentation and run some API tests to see what | ||
| happens. |
There was a problem hiding this comment.
This belongs to the very top of the blog post, you should explain people what they are going to do. It's promised that they learn how to contribute, but the core of the blog post starts here.
|
|
||
| Without the `opentelemetry.js` file that contains auto instrumentation, | ||
| triggering the `paymentservice/charge` API endpoint will result in no traces | ||
| showing up. Let’s reproduce this by running an API test with Tracetest. First, |
There was a problem hiding this comment.
Let me be frank: to me it feels unnecessary to use Tracetest here. It totally makes sense on the tracetest blog, it also makes sense to talk about tracetest in the previous blog post https://opentelemetry.io/blog/2023/testing-otel-demo/, but this blog post is titled "How to contribute to the otel demo", so stick to that topic, aka use Jaeger for visualization
| I’ll comment out all the content in the `opentelemetry.js` file. Then, comment | ||
| out all the OpenTelemetry-specific code in the `index.js` and `charge.js`. |
There was a problem hiding this comment.
Since you ask people above to take the code from a fork, this would be a place where you can shorten your blog post significantly. Make a version without the instrumentation and let people add it back in.
| 3. How to contribute to the OpenTelemetry Demo safely while avoiding | ||
| regressions. |
There was a problem hiding this comment.
where did I learn that? I would have expected some words around raising a PR, etc
|
Thanks a lot for the very detailed review @svrnm !! 🙌 You make great points. 🤝 I've summarized a todo list to work on:
|
That or you rename your blog post to something more fitting
Exactly, since this is mainly about tracing, if you need something for metrics take prometheus
Yes. And also try to keep the number of topics in the blog post "low". Right now there is a variety of things, that are only semi-related to the topic. Make cuts where possible.
You can use your fork throughout the doc if it adds value, but don't send people to a fork that in sync with upstream.
Exactly, ideally you put things in a special branch, since you might want to work with the demo in the future still.
Don't see this as a purely mechanical advice: this is the core of what you are talking about in this blog post, think about how you can lead with that and what is not part of that can be removed.
Yes.
Keep the things that are necessary for the flow of the blog, but otherwise point to the docs, yes. |
|
@adnanrahic any updates? |
|
Hey @svrnm! |
There was a problem hiding this comment.
Drive-by comment that this post will need a copyedit pass or two once the major rework has landed. /cc @theletterf
Thanks, I'll close this PR for now and you can raise a new one whenever you are ready! This makes it easier for us maintainers to keep all the open PRs visible. |
|
Sounds good. Sorry for the delay. I should be able to pick it back up next week since my conf presentation is this weekend. 😄 |
This PR adds an article that explains:
instrumentation.
functionality and telemetry integrity.
regressions.