-
Notifications
You must be signed in to change notification settings - Fork 138
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
[Shim] Add Auto-Collection Methods, TelemetryClient, and Config #1175
[Shim] Add Auto-Collection Methods, TelemetryClient, and Config #1175
Conversation
… setting the context.tags.
private _options: ApplicationInsightsOptions; | ||
private _client: AzureMonitorOpenTelemetryClient; | ||
private _console: AutoCollectConsole; | ||
private _exceptions: AutoCollectExceptions; | ||
private _idGenerator: IdGenerator; | ||
public context: Context; | ||
public commonProperties: { [key: string]: string }; // TODO: Add setter so Resources are updated | ||
public config: IConfig; | ||
|
||
/** | ||
* Constructs a new client of the client | ||
* @param setupString the Connection String or Instrumentation Key to use (read from environment variable if not specified) | ||
*/ | ||
constructor(input?: string | ApplicationInsightsOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove ApplicationInsightsOptions parameter from here and leave same structure as old Application Insights
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will break some things like the functional tests and the configuration example in the distro's README. Are we planning to release the shim after some changes to the distro's user configuration?
src/shim/telemetryClient.ts
Outdated
} | ||
|
||
public trackNodeHttpRequest(telemetry: Contracts.NodeHttpRequestTelemetry) { | ||
Logger.getInstance().warn("trackNodeHttpRequest is not implemented and is a no-op."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these not supported? what exactly these were doing before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They were some helper functions that called methods (trackRequestSync
and trackRequest
) inside of the AutoCollectHttpRequests
class. Given that OpenTelemetry tracks requests synchronously by default I figured it'd be less confusing to just say these methods weren't supported and have users unify on calling the single trackRequest
method.
I think it'd also be fair to implement them all calling the TelemetryClient.trackRequest()
or trackDependency
methods and just note that these will all be synchronous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update the message here to make it more explicit to users the expected alternative method to call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
setup()
andstart()
so customers can useclient.config
before runningstart()
config
properties to set corresponding values on the Azure Monitor Clientclient.config