feat: set OTEL global providers #28
Conversation
|
Well, I am not sure this is a good a idea to put it into a library: if any library would set the otel global variable, you would end up with a mess, I think. I would rather put those two lines of code in the krakend-ce itself. The effect would be the same, but krakend-ce is a binary and makes sense to set that value at the executable level. What do you think ? |
|
But IMHO only the part of KrakenD that creates meter/tracer providers should be the one setting them as global after creating them. If we move it into krakend-ce, doesn't it mean that krakend-ce becomes dependent on OTEL? But I never pushed code to krakend-ce, I don't know how much you want to keep things separate between the library and the app 😄 |
|
@dhontecillas, sorry for the ping. How should we proceed with this PR? Just wanted to keep the conversation going 😄 |
|
shouldn't we add the provider in the context? so that we can retrieve it from there? |
|
@TerraSkye, maybe, but it seems that having global providers is the way OTEL prefers it 🤷♂️ |
|
Unfortunately, this PR won't fix the #21 because the fqn of this package as a dependency of the binary and as a dependency of the plugin won't be the same, so the package variables won't be pointing to the same memory space |
|
@kpacha, but when I created a plugin, imported |
|
My plugin is more or less something like this: package main
import (
"crypto/tls"
"encoding/json"
"errors"
"reflect"
"github.com/go-redis/redis_rate/v10"
"github.com/krakend/krakend-otel/state"
"github.com/redis/go-redis/extra/redisotel/v9"
"github.com/redis/go-redis/v9"
)
type modifierRegisterer string
var (
ModifierRegisterer = modifierRegisterer(pluginName)
)
func (r modifierRegisterer) RegisterModifiers(f func(
name string,
factoryFunc func(map[string]interface{}) func(interface{}) (interface{}, error),
appliesToRequest bool,
appliesToResponse bool,
)) {
f(string(r), r.getModifyRequest, true, false)
}
func (r modifierRegisterer) getModifyRequest(
cfg map[string]interface{},
) func(interface{}) (interface{}, error) {
pluginCfg, ok := cfg[string(r)].(map[string]interface{})
if !ok {
logger.Fatal("Configuration not found")
}
m, err := r.getRateLimitRequestModifier(pluginCfg)
if err != nil {
logger.Fatal(err)
}
return m.ModifyRequest
}
func (r modifierRegisterer) getRateLimitRequestModifier(pluginCfg map[string]interface{}) (*RateLimitRequestModifier, error) {
// EXTRA STUFF
rdb := redis.NewClient(rdbOptions)
otelState := state.GlobalState()
// See https://stackoverflow.com/questions/13476349/check-for-nil-and-nil-interface-in-go
if otelState != nil && !reflect.ValueOf(otelState).IsNil() {
meterProvider := otelState.MeterProvider()
if err := redisotel.InstrumentMetrics(rdb, redisotel.WithMeterProvider(meterProvider)); err != nil {
return nil, err
}
tracerProvider := otelState.TracerProvider()
if err := redisotel.InstrumentTracing(rdb, redisotel.WithTracerProvider(tracerProvider)); err != nil {
return nil, err
}
}
limiter := redis_rate.NewLimiter(rdb)
m = &RateLimitRequestModifier{
limit: redis_rate.PerMinute(int(limitPerMinute)),
limiter: limiter,
skippedUserId: skippedUserId,
}
rateLimitRequestModifiers[cfgKey] = m
logger.Debug("Rate limit request modifier created")
return m, nil
} |
|
@adigiorgi-clickup that's weird, because that restriction was in place since the plugin package of the go stdlib was published. This conditioned the plugin loading and injecting for krakend and it is why we deal with local interfaces instead of sharing things like The most weird part for me is: that package was freeze several minors ago (aka several years) so there should be no code changes, so I'll need to check if anything else changed. I'll do a quick test to verify your statement. If the PR works, I'll be more than happy to accept it. Can you also share the go.mod of your plugin? That will help me with the recreation of your environment |
|
@kpacha, sure, here it is! |
|
Any update on this? |
|
@kpacha, please let me know if you need more info 🙏 |
Fixes #21