-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add gcom api processor. #44
Conversation
d94f23c
to
2c9b245
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with merging this as is, as I believe you tested and said this worked already. Everything I listed here can be done later on follow-up PRs.
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-gateway
distribution with this and the routing processor component. ( fa13512)