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

feat: poc: Telemetry architecture #169

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

feat: poc: Telemetry architecture #169

wants to merge 4 commits into from

Conversation

fabriziodemaria
Copy link
Member

@fabriziodemaria fabriziodemaria commented Nov 5, 2024

Note: the Demo App is configured to produce one typeMismatch error when tapping the button in the UI.

Resulting monitoring payload:

{
  "countTraces": [
    {
      "traceId": 2,
      "count": 0
    },
    {
      "count": 1,
      "traceId": 1
    }
  ],
  "libraryId": 13,
  "libraryVersion": "1.0.1",
  "durationsTraces": [] // Mean/Avg by traceId?
}

(est. 140 bytes JSON and 17 bytes Proto)

TODO:

  • Basic scaffolding and payload structure
  • Implement at least one trace
  • Add sdk id and sdk version
  • Discuss transport layer, consider payload size (protobuf encoded header in JSON)
  • Track upstream wrappers (i.e. OpenFeature): deal with it now or later? (ok for later, try fix it now)
  • Lost data: network not often reached throughout a session (in static-context) (add it to apply)
  • Do we need a debug mode to filter out telemetry from dev builds? (maybe expose debug mode setter to users. Not include in this PR)
  • Protobuf data
  • Testing setup for TelemetryManager
  • Coverage and linitng

if let header = header {
let jsonHeaderData = try encoder.encode(header)

if let headerJsonString = String(data: jsonHeaderData, encoding: .utf8) {
Copy link
Member

Choose a reason for hiding this comment

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

Knowing that this is in a POC stage but leaving the comment to spark ideas: Most likely we don't want to use json as it's too verbose. Not necessarily saying that protobuf can be an option here (because of potential binary size bloat). We need something that works cross platform and is small, without a big footprint on the SDK.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would stay away from a custom protocol and rather embrace protobuf fully, if we want to go that route.
I also hope that we can limit the payload size in JSON by using numeric identifiers rather than strings, but this is TBD

Copy link
Member Author

Choose a reason for hiding this comment

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

My opinion is to aim to wrap this PR up with JSON, and tackle this discussion in a future PR is necessary

do {
let result: HttpClientResult<ResolveFlagsResponse> =
try await self.httpClient.post(path: ":resolve", data: request)
try await self.httpClient.post(path: ":resolve", data: request, header: header)
Copy link
Member

Choose a reason for hiding this comment

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

What does this header param in the post function accept?
IMO the header is pretty likely to be a key-value "tuple" that's gonna be appended to other headers.
If that's the case we probably want to have a common header key X-CONFIDENCE-TELEMETRY and pass the full telemetry data as the value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am concerned about using the header in general, as it might be handled differently by various proxies and might have different and more unpredictable size limitations. We would also create new code paths in the backend to deal with data in the header (minor point, but still worth mentioning in the tradeoff discussions)

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.

2 participants