-
Notifications
You must be signed in to change notification settings - Fork 53
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
C API improvements. Host-Guest improvements. Ability to update context via C API. #1122
Open
maxgolov
wants to merge
56
commits into
main
Choose a base branch
from
maxgolov/c_api_context
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…metry into maxgolov/shared_library
…t/cpp_client_telemetry into maxgolov/shared_library
…metry into maxgolov/shared_library
…metry into maxgolov/shared_library
…t/cpp_client_telemetry into maxgolov/shared_library
…Key specified on event
…e version to current v3.7
maxgolov
requested review from
lalitb,
sid-dahiya and
ThomsonTan
as code owners
March 30, 2023 06:02
maxgolov
commented
Mar 30, 2023
maxgolov
commented
Mar 30, 2023
- Fix Host-Guest feature bugs. - Add more tests for Host-Guest feature.
…/cpp_client_telemetry into maxgolov/c_api_context
…/cpp_client_telemetry into maxgolov/c_api_context
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc: @kindbe since we used to work originally on this feature, especially the
scope
parameter.Description
There is a very detailed long-overdue markdown document that explains the Host-Guest feature, use-cases, purpose of
scope
(for sandboxing / isolation). And a separate file with fairly comprehensive Functional Tests (TADA!).What it gives - TL;DR
Our team uses 1DS C API to pass telemetry contexts around in our hybrid app that is written in Unity C#, C/C++ and JavaScript (WebAssembly). Eventing works great. What we are missing is ability to
consolidate telemetry contexts
across languages. 1DS itself has theSetContext
API andSemantic Context
concept. Although these APIs were not exposed to C API. And did not have ability to intake parameters expressed in canonic Common Schema notation. Ex. you could not passext.device.localId
orext.os.ver
toSetContext
. Our team also uses exclusively the modern Common Schema. While old-styleCOMMONFIELD
aliases could be appropriate for teams that export to A**a-Kusto, in our case we prefer our developers to operate on classic Common Schema events. The way you see them showing in Diagnostic Data Viewer application. Referencing old-style aliases causes confusion, questions, and unnecessary effort to remember things our devs would hardly ever need.What's included:
evt_set_logmanager_context(s)
andevt_set_logger_context(s)
. These APis are somewhat smarter thanSetContext
since they'd automagically detect a Common Schema Part A property if it starts withext.
. And remap it to corresponding place in BondCsRecord
. This would also be a gateway to eventual adoption of JSON notation: more and more customers move away from A**a-Kusto to their own private Kusto clusters - due to privacy, compliance, GDPR and EUDB requirements, that could be more easily satisfied with a private cluster. All these customers would prefer Common Schema notationext.app.localid
instead ofAppInfo.Id
. Also this matches precisely what you see on wire.This PR also contains a stable C# ABI from my other older PR. I'll refactor the other one to use it as a C# usage example instead. API has been tested and proven to work on Windows, Mac, and Android ARM64 devices.