Support performance trace #32973
Support performance trace #32973
Conversation
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/L
github-actions
bot
added
modifies/translation
modifies/go
wxiaoguang
force-pushed
the
support-trace
branch
6 times, most recently
from
bd47ebb to
4b7547b
Compare
wxiaoguang
force-pushed
the
support-trace
branch
from
4b7547b to
ee7a30e
Compare
# Conflicts: # modules/web/routing/context.go
|
Background: OpenTelemetry is cool for k8s, or users who have setup a complete infrastructure. For many small to medium instance users who only use single binary installation or docker-compose installation, it seems too heavy for them to install OpenTelemetry eco-system tools to collect performance reports. This PR introduces an OpenTelemetry-like "tracer" shim-layer and introduces a simple builtin tracer, then every site admin could open their admin panel to find the performance trace logs and report them to Gitea's issue tracker to help to resolve the performance problems. For advanced users, they could also introduce the OpenTelemetry tracer by implementing "traceStarter interface" (just like Discussions could go to #32866 screenshots
Sample: |
github-actions
bot
added
the
modifies/cli
wxiaoguang
force-pushed
the
support-trace
branch
from
82f8fd7 to
7b64951
Compare
wxiaoguang
force-pushed
the
support-trace
branch
from
7b64951 to
5572bbe
Compare
It is only used internally, end users won't see it. |
|
It seems that I can't track the data being migrated, or maybe I don't understand this feature well enough. |
|
I did see the logger system. I do find it impressive and I mean this as a compliment - I tried to port to to I do however question the actual need here. There's no legacy system to swap, noop is built-in and default if no tracer is configured. API isn't that big and it's a standard. I would have one last argument for using the otel API but I don't think any library with uses it is being included so it's dead on arrival: Integrating with the api would allow for cross library traces to be captured.
That sounds like "no" to me though. I don't understand why can't we use the api to our advantage over putting a shim, over an abstraction. So I'm on the opposite end of the argument and on a losing position because of 500KB (otel api size) at this point. Ultimately, I do think that this implementation will end up in main regardless of how much I'll oppose the approach here and I'll end up having to have an out of tree patch with otel... |
This PR is only an initial. To trace a system correctly, it needs a lot of fine-tune works. It's impossible to make the trace system work well by just adding a 3rd library. To answer your question:
These improvements should be done in the future. |
I think it's not in Gitea's current scope because there is no cross-library calls in Gitea. Otel is well-designed for micro-services in K8s. If one day Gitea becomes large enough and need to cross-trace between services, we could simple drop the |
I think I might've phrased it wrong. Suppose xorm has an otel integration (it doesn't but let's assume this for arguments sake). By plugging into otel system you can trace xorm operations directly, which I believe merging this will close my proposal as "not planned". |
To be honest, I do not think (and seldom see) any general library would really integrate otel by default -- otel is really good for a in-house system, but not for a general library. The otel package itself isn't in a stable stage yet -- changes a lot. If otel wins in the future (in almost every library), we could also just integrate it without any concern. But I guess we won't see it in near future. |
Well, my proposal is to add otel as a "trace plugin" (the |
I don't insist on dropping built-in tracer at all! In fact the fact it works is really impressive and a lot better than what I tried to do with custom exporter. And it is a really great idea! |
|
I think we have a traditional to have an abstract layer for third-party libraries. This will make it easier to introduce another tracer plugin that has a different interface from OTEL. If we use OTEL's interface, it will lose the flexibility. I think the current design will not prevent the use of OTEL's plugins for a OTEL implementation. |
Hmm .... maybe because that when I read your branch, I see it removes the builtin tracer to add otel, it misleads me. It's not quite clear to me about what's your idea to make the builtin and otel tracers work together. My proposal is to just write Anyway, I think this PR won't block the otel's integration, I think we can move on and try to figure out a best solution step by step. |
Well... that's fair. Unfortunate for me but fair.
Then to make it clear what my idea was - my proposal is to have internal tracer because it's a great idea, but register it as a otel provider. I'm not even arguing for adding sdk and exporters because you're absolutely right that they weigh a lot and most users would consider it bloat instead of a feature. In semi valid golang// Skipping imports
// Most useful line here, which isn't the greatest idea to do in init but it will do until external exporters would be added
func init(){
otel.SetTracerProvider(NewInternalProvider())
}
type GiteaInternalTracerProvider struct {embedded.TracerProvider ,tracer GiteaInternalTracer}
type GiteaInternalTracer struct {embedded.Tracer} // Which is basically Tracer already in place
type GiteaInternalSpan struct {noop.Span} // Which is basically your span implementation without the need for internal spans. Noop is added so every non defined method is automatically a noop
// Create new provider
func NewInternalProvider() trace.TracerProvider{
return GiteaInternalTracerProvider{tracer: GiteaInternalTracer{}}
}
// Return tracer
func (itp GiteaInternalTracerProvider) Tracer(name string, options ...trace.TracerOption) trace.Tracer{ return it.tracer }
func (it GiteaInternalTracer) Start(ctx context.Context, spanName string, opts ...trace.SpanStartOption) (context.Context, trace.Span){
// Current Start func
}
Worst case scenario I'll maintain a patch for myself. I'll complain about it but I can't do much besides that. |
GiteaBot
added
lgtm/need 1
GiteaBot
added
lgtm/done
lunny
added
the
reviewed/wait-merge
GiteaBot
removed
the
reviewed/wait-merge
This PR's design is quite lightweight, no hard-dependency, and it is easy to improve or remove. We can try it on gitea.com first to see whether it works well, and fine tune the details.