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: Add builtin OpenTelemetry support #33908

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

Conversation

wxiaoguang
Copy link
Contributor

It works.

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 16, 2025
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 16, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Mar 16, 2025
@wxiaoguang wxiaoguang marked this pull request as draft March 16, 2025 12:54
@TheFox0x7
Copy link
Contributor

Ah so this is what you had in mind...
Can't say I'm happy about this idea but we went over this multiple times already.
Couldn't this at least use regular library behind compilation flag to not have the binary size issue?

@wxiaoguang
Copy link
Contributor Author

Couldn't this at least use regular library behind compilation flag to not have the binary size issue?

Well, this is just a demo. Actually I was trying to add a build flag to introduce the otel tracer, but then I found that the JSON API is officially supported and extremely simple. https://github.com/open-telemetry/opentelemetry-proto/blob/main/examples/trace.json . Then I think this could be a good solution.

@TheFox0x7
Copy link
Contributor

Well in that case, thanks for the technical discussions on the feature then - it was a really fun time. Shame we have different ideas and needs for this feature.
I'll let you work on this without my interference.

@techknowlogick
Copy link
Member

@TheFox0x7 I think this approach can support introducing the official otel library and have it behind a build flag so as not to introduce the additional size by default

@TheFox0x7
Copy link
Contributor

I see no point of adding it in general if this approach will be built-in.

Full disclosure:
I'd be lying if I said I'm not dissatisfied and annoyed by the outcome of my feature request and that my idea for it got thrown into away but I can't do anything expect have my patches swapping the system around on top of gitea and maintain them on my own.
I cannot argue with the reasoning behind it - both dependency size issue (so no SDK by default) and the abstraction layer requirement (so no direct API usage). I may not like it but it is what it is and I have to accept that this is the default implementation over what I wanted.

@github-actions github-actions bot added modifies/cli PR changes something on the CLI, i.e. gitea doctor or gitea admin docs-update-needed The document needs to be updated synchronously labels Mar 17, 2025
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 17, 2025

I'd be lying if I said I'm not dissatisfied and annoyed by the outcome of my feature request and that my idea for it got thrown into away but I can't do anything expect have my patches swapping the system around on top of gitea and maintain them on my own.

That's still the unclear part to me: why this design doesn't satisfy your requirement, and why you need to write your own otel code and maintain your own. Till now, the otel sdk just doesn't work with Gitea due to various legacy problems (including the ctx problem). I do not think you are able to make otel sdk really work correctly and collect what you want in current code base.

Everything I am doing now is trying to figure out how to make Gitea could report its traces to otel collector correctly, and as I said, it won't block anything including using the official sdk in the future.


If this design blocks anything, please show a real example to help me to understand the problem.

@wxiaoguang
Copy link
Contributor Author

I'd be lying if I said I'm not dissatisfied and annoyed by the outcome of my feature request and that my idea for it got thrown into away but I can't do anything expect have my patches swapping the system around on top of gitea and maintain them on my own.

By the way, TBH I don't want to disappoint you .... and thank you very much for helping discussing and resolving various problem. Actually if you have enough time to help users to diagnose these deadlock/slow problems, it's indeed very appreciated and you could propose related changes for these problems and put the ideas into practice.

@TheFox0x7
Copy link
Contributor

TheFox0x7 commented Mar 17, 2025

Till now, the otel sdk just doesn't work with Gitea due to various legacy problems (including the ctx problem) [...]
I do not think you are able to make otel sdk really work correctly and collect what you want in current code base.

It's the main reason why I try to detach context from places actually. And my latest attempt actually works way better :)

To be honest it comes down to preference. I hold the opinion that the design would be better if it relied on the otel API instead of wrapping it but the argument for wrapping is not something I can fight with anything but "I don't like it". That's a poor argument and to date I lack better ones.

By the way, TBH I don't want to disappoint you .... and thank you very much for helping discussing and resolving various problem. Actually if you have enough time to help users to diagnose these deadlock/slow problems, it's indeed very appreciated and you could propose related changes for these problems and put the ideas into practice.

Please don't take what I wrote personally. You had better arguments for your approach and I respect it. Maybe I could've done better with my side. And I really enjoyed the discussions :)
I still intend to cooperate on this as we'll use the base idea in the same way - just I'll have the API instead of the wrapper.


Speaking of which the database spans need to go IMO. To explain why go to activity/yearly on a larger repo (tried on linux) and wait for it to arrive in jaeger.

it ended up being around ~4500 spans, mainly database ones which is useless.

@wxiaoguang
Copy link
Contributor Author

Speaking of which the database spans need to go IMO. To explain why go to activity/yearly on a larger repo (tried on linux) and wait for it to arrive in jaeger.

it ended up being around ~4500 spans, mainly database ones which is useless.

It seems that you caught this issue: When there are too many Commits, the "Contributors" page displays an "Internal Server Error." #33867 🤣

@TheFox0x7
Copy link
Contributor

not that endpoint but partially yeah.

But activity does return what it should in ~2 minutes so it's just dos'ing my jaeger UI db spans, all of which are get me the user with different mail

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-update-needed The document needs to be updated synchronously 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/go Pull requests that update Go code size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants