open-telemetry / opentelemetry-go-contrib

Collection of extensions for OpenTelemetry-Go.
https://opentelemetry.io/
Apache License 2.0
1.2k stars 563 forks source link

example/dice: Make sure that all code is available for docs and easy to consume for end users #6296

Open svrnm opened 2 months ago

svrnm commented 2 months ago

Description

I wanted to re-do https://github.com/open-telemetry/opentelemetry.io/pull/4712, and reviewed the code and the documentation to figure out how to match both, while doing so I realized that the dice example in the repo is much more complicated than the one we have in the documentation, i.e. when I get to the step "Instrument the HTTP server", the HTTP server suddenly comes with SIGINT handling, shutdown handling, a much more complex http server setup etc, so we go from

package main

import (
    "log"
    "net/http"
)

func main() {
    http.HandleFunc("/rolldice", rolldice)

    log.Fatal(http.ListenAndServe(":8080", nil))
}

to https://github.com/open-telemetry/opentelemetry-go/blob/main/example/dice/main.go in one jump.

Also, the non-instrumented file is not sourced from the repo.

Steps To Reproduce

Review https://opentelemetry.io/docs/languages/go/getting-started/

Expected behavior

To make this work on both ends (docs + go repo) we need to have a clear agreement on the building blocks, from my point of view the following should exist in the go repo in the example/dice folder:

Ideally the code is as simple as possible for all things not otel related. I am not a go expert, so I will leave it to the SIG Go members, to decide what part of the boilerplate code is required.

Even, better if we even can have the installation steps also included in the folder, e.g. shell files for:

This way we could even verify in a test if all steps can be executed without exceptions (extra plus plus if we do some testing if data is emitted)

cc @open-telemetry/docs-approvers

pellared commented 2 months ago

the HTTP server suddenly comes with SIGINT handling, shutdown handling,

Ideally the code is as simple as possible for all things not otel related. I am not a go expert, so I will leave it to the SIG Go members, to decide what part of the boilerplate code is required.

I just want to share that I find these boilerplate important as certain (quite big IMO) amount of people do shutdown handling improperly.

Probably the easiest way forward would be to have one directory without OTel and second with OTel. @svrnm, could this work?

svrnm commented 2 months ago

the HTTP server suddenly comes with SIGINT handling, shutdown handling,

Ideally the code is as simple as possible for all things not otel related. I am not a go expert, so I will leave it to the SIG Go members, to decide what part of the boilerplate code is required.

I just want to share that I find these boilerplate important as certain (quite big IMO) amount of people do shutdown handling improperly.

I am not the SME here, so if you decide to have it, I am OK with it, we just need to have it in both

Probably the easiest way forward would be to have one directory without OTel and second with OTel. @svrnm, could this work?

Yes, that would work.

jeffreylean commented 1 month ago

Hi, I would like to take on this task, just to be clear, the solution is to create another "version" of the example/dice codebase without OTel right ?

svrnm commented 1 month ago

@jeffreylean, yes, 2 folders, with the app instrumented&uninstrumented.

please give it a try:-)

IgorEulalio commented 1 month ago

@jeffreylean I'd love to help you on this task, I'm looking forward to start contributing on OpenTelemetry, I've sent you a message.