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

otel: simple OTEL collector/Prometheus stack for testing purposes #5026

Merged
merged 1 commit into from May 2, 2024

Conversation

krissetto
Copy link
Contributor

@krissetto krissetto commented Apr 17, 2024

- What I did
Added a compose stack to be used for testing out OTEL functionality, mostly based on our current documentation PR found here.

- How I did it

  • Added compose stack;
  • Added basic OTEL collector and prometheus configs;
  • Added basic usage doc

- How to verify it
Read the doc and try it out :)

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@krissetto krissetto self-assigned this Apr 17, 2024
Copy link
Member

@Benehiko Benehiko left a comment

Choose a reason for hiding this comment

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

I'm not sure about this change since it introduces more complexity with external shell scripts. I'd prefer to keep everything inside Go.

Could we not use testcontainers for this instead? :)

@thaJeztah
Copy link
Member

Slightly wondering if we need the scripts at all, as they (effectively) are just a docker compose up?

(I know we have a bunch of ahack/ script already though, but mostly looking if for this one we need more)

@laurazard
Copy link
Member

IMO a big part of why I want this change is to make local development easier, so Testcontainers don't really achieve that. I'm okay with shell scripts are they're portable and easy to work with and don't require bringing in additional dependencies, but if atm they are really just doing a compose up, compose down, etc., then I'm okay with holding off on them and just adding the Compose file, and thinking about the rest later if we feel the need for them.

@thaJeztah
Copy link
Member

Yes, agreed on the "local development" part. It's usually also my reservation on things like mage as replacement for make; it's cool as it's in Go (and can be flexible), but also means that go is now a requirement on your local machine (and where possible, we should try to still allow for a dockerized workflow, so that there's no real requirements other than "have docker installed")

Copy link
Member

@laurazard laurazard left a comment

Choose a reason for hiding this comment

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

LGTM, with a suggestion to remove the shell scripts.

@krissetto
Copy link
Contributor Author

I'm not super fond of the scripts either, I was trying to follow a bit of what already existed while keeping things as simple s possible for the community (eg. i think buildx has a script which completely wraps compose, for example, and i thought that was a bit too much).
If we agree on removing them completely, I'll adjust the usage.md to reflect that.

@krissetto krissetto force-pushed the otel-test-stack branch 2 times, most recently from 628ae1f to c91c271 Compare April 19, 2024 10:01
@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.05%. Comparing base (b982833) to head (c91c271).
Report is 28 commits behind head on master.

❗ Current head c91c271 differs from pull request most recent head e1dcc19. Consider uploading reports for the commit e1dcc19 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5026   +/-   ##
=======================================
  Coverage   61.05%   61.05%           
=======================================
  Files         295      295           
  Lines       20647    20647           
=======================================
  Hits        12605    12605           
  Misses       7146     7146           
  Partials      896      896           

Copy link
Member

@Benehiko Benehiko left a comment

Choose a reason for hiding this comment

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

LGTM

@krissetto
Copy link
Contributor Author

ps i added aspire dashboard here as well so we can test both and see what we want to stick with

hack/otel/usage.md Outdated Show resolved Hide resolved
hack/otel/usage.md Outdated Show resolved Hide resolved
hack/otel/usage.md Outdated Show resolved Hide resolved
Signed-off-by: Christopher Petito <chrisjpetito@gmail.com>
Comment on lines +16 to +17
# Mount the prom.yml config file
- ./prom.yaml:/etc/prometheus/prom.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker for this PR, but I know that bind-mounting files can sometimes be problematic, so looking if we could have a directory to mount (which contains the config-file). Looks like /etc/prometheus already has some files in the image though, so if we do, we probably should use a different location for the config-file (but good news is that we already define it as a custom location above, so ... could still be an option);

docker run --rm --entrypoint /bin/sh prom/prometheus:latest -c 'ls -l /etc/prometheus'
total 4
lrwxrwxrwx    1 nobody   nobody          39 Apr 10 14:30 console_libraries -> /usr/share/prometheus/console_libraries
lrwxrwxrwx    1 nobody   nobody          31 Apr 10 14:30 consoles -> /usr/share/prometheus/consoles/
-rw-r--r--    1 nobody   nobody         934 Apr 10 14:21 prometheus.yml

The alternative could be to define it as a config: in the compose-file (but those are still implemented as bind-mounts in compose, so won't solve the "don't bind individual files" part)

Comment on lines +34 to +36
volumes:
# Mount the otelcol.yml config file
- ./otelcol.yaml:/etc/otelcol/config.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Same for this one (also not a blocker)

@thaJeztah thaJeztah merged commit 647ccf3 into docker:master May 2, 2024
103 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants