Add gcom api processor. #44
Conversation
|
|
kovrus
force-pushed
the
gcom-api
branch
3 times, most recently
from
d94f23c
to
2c9b245
Compare
kovrus
force-pushed
the
gcom-api
branch
2 times, most recently
from
3147035
to
32f4c7c
Compare
|
|
|
|
||
| "github.com/go-kit/log" | ||
| "github.com/go-kit/log/level" | ||
| "github.com/grafana/opentelemetry-collector-components/processor/gcomapiprocessor/internal/gcom/client" |
There was a problem hiding this comment.
we might need a linter to enforce three import groups -- this one here would belong to a third group, of local imports
|
|
||
| // CheckKey checks if the provided key is valid | ||
| func (a *AuthCache) CheckKey(ctx context.Context, key string) (*client.APIKey, error) { | ||
| span, _ := opentracing.StartSpanFromContext(ctx, "Gateway.CheckKey") |
There was a problem hiding this comment.
I know which code I'll change from opentracing to otel first :-)
|
|
||
| type AuthCache struct { | ||
| client client.Client | ||
| logger log.Logger |
There was a problem hiding this comment.
The collerctor uses zap instead -- does it make sense to switch to it at a follow-up PR?
| } | ||
|
|
||
| // RegisterFlagsWithPrefix adds the flags required to config this to the given FlagSet | ||
| func (cfg *InstanceCacheConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) { |
There was a problem hiding this comment.
Are those flags actually used?
|
|
||
| if len(instanceTypes) == 0 { | ||
| // TODO (@chroberts): replace this with an error after we roll out | ||
| // https://github.com/grafana/backend-enterprise/pull/3749 |
There was a problem hiding this comment.
This is merged -- should this code be changed already?
| } | ||
|
|
||
| func (i *instanceCache) run() { | ||
| completeRefreshTicker := time.NewTicker(i.cfg.CompleteCacheRefreshDuration) |
There was a problem hiding this comment.
Ideally, this would receive a context, which would be cancelled when the component is shutting down. In that case, the for range channel would be replaced with selects, so that we can cancel the cache refresh operations when we are shutting down.
|
|
||
| res, err := c.client.Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("err=%w, msg=%v", ErrAccess, err) |
There was a problem hiding this comment.
Are we leaking internal details with this message? Or do we trust that the internal gateway is returning only information that is safe for the general public?
| ) | ||
|
|
||
| var ( | ||
| grafanaComReqs = promauto.NewHistogramVec( |
There was a problem hiding this comment.
Do we want to use the telemetry settings that the collector provides? While the registries might end up merged, this is likely to change in the future.
| } | ||
|
|
||
| // RegisterFlags adds the flags required to config this to the given FlagSet | ||
| func (cfg *Config) RegisterFlags(f *flag.FlagSet) { |
There was a problem hiding this comment.
Should we remove this?
| } | ||
|
|
||
| // New creates a grafana.com Client | ||
| func New(cfg Config, name string, logger log.Logger) (Client, error) { |
There was a problem hiding this comment.
I just realized that those are the two packages you said you just copied from the enterprise package. If we are going to maintain them, should we accomodate them to the collector standards? Or do we want to threat those two packages as libraries? In that case, it would probably make sense to split them into different go modules perhaps.
There was a problem hiding this comment.
I'll follow up with a PR against backend-enterprise to convert those packages to modules. If it wouldn't work for some reason, let's improve the code and strip all unnecessary parts.
|
I've addressed all the comments except comments on the |
See Design Doc: Use OTel Collector as a replacement for OTLP Gateway in Grafana Cloud OTLP Endpoint
for more information on why this component is needed.
Note: I've copied only cache and client modules from
https://github.com/grafana/backend-enterprise. I did not use it as dep because it causes some des conflicts and it has to be fetched from private repo (which might be ok actually).Follow up:
otlp-gatewaydistribution with this and the routing processor component. ( fa13512)