Skip to content

Conversation

@cwaldren-ld
Copy link
Contributor

@cwaldren-ld cwaldren-ld commented May 3, 2023

This PR generates feature events when Variation methods are called. I've also added detailed evaluations.

There is still work to be done here, mainly in:

  • refactoring VariationInternal (it's long, and pretty messy)
  • deciding if we want to keep the converters I've added to Value to facilitate conversion to primitive types.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #199609: Generate analytic events.

@cwaldren-ld cwaldren-ld force-pushed the cw/sc-199609/generate-analytic-events branch from c6b6af1 to 9ac9564 Compare May 3, 2023 01:34
<< "Value was: " << detail_val.first;
}
std::this_thread::sleep_for(std::chrono::seconds(1));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need main_casey.cpp and main_ryan.cpp

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we will eventually accidentally make a useful app.

// TODO: SC-199918
auto error_reason = EvaluationReason("CLIENT_NOT_READY");
if (eval_reasons_available_) {
event.reason = error_reason;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm copying the if (reasons) part from the Go client SDK, but I'm not sure the justification and I might have it wrong - will check other SDKs. The spec says:

The reason property is only set if any of the following is true:

    The application used the VariationDetail operation rather than Variation.

    For server-side SDKs, any of the conditions described in the previous “for server-side SDKs” bullet list are true.

    For client-side SDKs, the flag’s trackReasons property is true.

So I'd think it should only be set if flag.track_reason() or if detailed is true. Now, we can't use the former because the flag isn't present, so then it's up to detailed. But the Go SDK is using the fact that the user requested withReasons...

return std::nullopt;
}

auto events = boost::json::value_from(outbox_.Consume());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a bug; it wouldn't send any summary event if the outbox was full. Now it properly sends summary events if they exist.

@cwaldren-ld cwaldren-ld marked this pull request as ready for review May 3, 2023 23:15
@cwaldren-ld cwaldren-ld requested a review from kinyoklion May 3, 2023 23:16
namespace launchdarkly {

template <typename T>
class EvaluationDetail {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have some docs on this type, being as it is returned by the public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point.

@cwaldren-ld cwaldren-ld merged commit c62dcf6 into main May 4, 2023
@cwaldren-ld cwaldren-ld deleted the cw/sc-199609/generate-analytic-events branch May 4, 2023 21:12
This was referenced Oct 23, 2023
@github-actions github-actions bot mentioned this pull request May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants