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

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

Open
svrnm opened this issue Sep 12, 2024 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@svrnm
Copy link
Member

svrnm commented Sep 12, 2024

Description

I wanted to re-do open-telemetry/opentelemetry.io#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:

  • an not-yet-instrumented version of the code
  • an instrumented version of the same code. Only difference should be the otel code.

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:

  • go mod init dice
  • go run .
  • `go get "go.opentelemetry..."
  • go mod tidy; export OTEL_RESOURCE_ATTRIBUTES="service.name=dice,service.version=0.1.0"; go run .

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

@svrnm svrnm added the bug Something isn't working label Sep 12, 2024
@pellared pellared added enhancement New feature or request and removed bug Something isn't working labels Sep 12, 2024
@pellared
Copy link
Member

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
Copy link
Member Author

svrnm commented Sep 12, 2024

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.

@pellared pellared added help wanted Extra attention is needed good first issue Good for newcomers labels Sep 12, 2024
@jeffreylean
Copy link

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
Copy link
Member Author

svrnm commented Sep 24, 2024

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

please give it a try:-)

@IgorEulalio
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants