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

Support reload_interval in remote config source mode #24840

Closed
zcross opened this issue Aug 3, 2023 · 2 comments
Closed

Support reload_interval in remote config source mode #24840

zcross opened this issue Aug 3, 2023 · 2 comments

Comments

@zcross
Copy link
Contributor

zcross commented Aug 3, 2023

Component(s)

extension/jaegerremotesampling

Is your feature request related to a problem? Please describe.

As mentioned in the jaegerremotesampling extension's existing README, it would be nice to have optional (and configurable) collector-side caching behavior when the extension is used in conjunction with a remote gRPC endpoint implementing the JaegerRemoteSampler config API:

At this moment, the reload_interval option is only effective for the file source. In the future, this property will be used to control a local cache for a remote source.

Why is this nice? It would reduce collector -> remote endpoint volume within the configured caching window, for collector operators who care about things like egress or about the load on the remote backend endpoint. And it should be fully optional, so opting into the setting involves conscious acceptance of the eventual consistency implications for remote strategy updates (cached data is stale within TTL periods).

Describe the solution you'd like

I'd like to use the same language (and field even) for reload_interval, even if for the gRPC remote interaction it is slightly different (given that requests already specify a service name). I recommend TTL-based in-memory caching, where the TTL for each cached strategy item is just set to the reload_interval. That seems pretty intuitive for collector owners at a high level, and matches the language of the file mode of operation (which already supports reload_interval).

Describe alternatives you've considered

No response

Additional context

For what it's worth, I already have a branch that was based on #24414 and just needs some rebasing/cleanup. It has integration test coverage and unit testing of the cache, but I'll probably do quite a bit more to bring it up to par for code review. For now, I'm holding off on opening a PR with that branch because it's stacked on the branch of #24414 and I don't really know how to do stacked PRs with forks involved.

My work in progress and soon-to-come-via-PR branch: https://github.com/zcross/opentelemetry-collector-contrib/tree/origin/zcross/jaegerremotesampling/support-remote-policy-ttl-caching

@zcross zcross added enhancement New feature or request needs triage New item requiring triage labels Aug 3, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@zcross
Copy link
Contributor Author

zcross commented Aug 21, 2023

Whoops, this is a little late: this was released in 0.83.0 via #24981

@zcross zcross closed this as completed Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants