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

WIP: Init OpenTelemetry Support #32971

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

techknowlogick
Copy link
Member

Add otel support to Gitea. Includes DB & HTTP request spans. Tested with honeycomb and uptrace.

Marked as WIP, this is more for discussion purposes and possibly contributing to a future feature than anything else. There are several flaws with this PR as it currently exists, the first being that it essentially hardcodes using the HTTP collector vs allowing GRPC as an alternative and that the settings to configure this are entirely env var-based rather than using an existing pattern for settings config in the app.ini file. More on that last point, since the configuration is left to env vars, this is essentially always on, and with no config, will be screaming into the void, possibly at a service listening on the localhost port (this is less than ideal, to say the least). There are also no tests for this PR, which again, is less than ideal, although this PR purpose leads tests for it to be unnecessary, as it is unlikely to be merged in this state (

If this were to go anywhere, collecting some span/traces for shelling out would be nice and possibly adding tracing for Redis/Memcached, too, if needed.

From testing this PR, I've already identified a few hotspots of areas that could receive extra attention, but those will be collected later.

Related Issue: #32866

@techknowlogick techknowlogick added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Dec 24, 2024
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 24, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 24, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/dependencies labels Dec 24, 2024
@TheFox0x7
Copy link
Contributor

Few comments based on my work:

first being that it essentially hardcodes using the HTTP collector vs allowing GRPC as an alternative

Completely fine by spec.

If they support only one, it SHOULD be http/protobuf

More on that last point, since the configuration is left to env vars, this is essentially always on, and with no config, will be screaming into the void, possibly at a service listening on the localhost

Yeah, unfortunately golang sdk does not have disable support or any of the individual ones.

If this were to go anywhere, collecting some span/traces for shelling out would be nice and possibly adding tracing for Redis/Memcached, too, if needed.

Env vars are - as much as they are a great idea - dead end in gitea if we want to have hook/serv command covered. In docker this would work but from a command line I don't think it will as you'd have to set env vars for them too, which unless I missed something would be a lot of trouble for not a lot of gain.
Moving this to settings would be better and work for all components (at the cost of having an exporter start per command which is also terrible)

Ideally the command would send it's data to main server which would forward it to the exporter - no idea how to do this yet because I just came up with this.

On env var and settings topic - if we'd support OTEL_* env vars instead of GITEA__opentelemetry__* for configuring this it would be best IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin modifies/dependencies modifies/go Pull requests that update Go code pr/wip This PR is not ready for review size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants