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

[flagd] ContextEnrichment for inprocess improvements by extracting logic onto the channelmonitor #1231

Open
aepfli opened this issue Feb 19, 2025 · 1 comment
Labels
Needs Triage This issue needs to be investigated by a maintainer

Comments

@aepfli
Copy link
Member

aepfli commented Feb 19, 2025

Currently our Context Enrichment is handled here:

Exception metadataException = null;
log.debug("Initializing sync stream request");
final GetMetadataRequest.Builder metadataRequest = GetMetadataRequest.newBuilder();
GetMetadataResponse metadataResponse = GetMetadataResponse.getDefaultInstance();
try (CancellableContext context = Context.current().withCancellation()) {
try {
metadataResponse = grpcConnector.getResolver().getMetadata(metadataRequest.build());
} catch (Exception e) {
// the chances this call fails but the syncRequest does not are slim
// it could be that the server doesn't implement this RPC
// instead of logging and throwing here, retain the exception and handle in the
// stream logic below
metadataException = e;
log.debug("Metadata exception: {}", e.getMessage(), e);
}

We always try to fetch the metadata before we reestablish a connection, during our stream listening logic.

This might be a little bit tricky, as we are always relying on the same process, and if eg. the timeline exceeds etc. the whole process breaks.

The question is how important it is to fetch this metadata, as we could theoretically also do this unary call when the channel state changes to READY, instead of always during the loop.

This would decouple our sync from the metadata, and might increase our robustness.

WDYT?

cc: @guidobrei @toddbaert @beeme1mr @alexandraoberaigner @chrfwow

@aepfli aepfli added the Needs Triage This issue needs to be investigated by a maintainer label Feb 19, 2025
@toddbaert
Copy link
Member

toddbaert commented Feb 19, 2025

Another option I was thinking about is that we could deprecate this RPC entirely, and add it to the content of the sync itself... so instead of the SyncFlagsResponse just having a flagConfiguration field, it could also have a syncMetadata field (or perhaps we could rename it something more in-line with its usage, like staticContext or something)...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Triage This issue needs to be investigated by a maintainer
Projects
None yet
Development

No branches or pull requests

2 participants