AppInsightTelemetry support for CNI #458
Conversation
telemetry/aiwrapper.go
Outdated
| var ( | ||
| aiMetadata string | ||
| th aitelemetry.TelemetryHandle | ||
| gDisableAll bool |
There was a problem hiding this comment.
what's the g prefix for?
There was a problem hiding this comment.
g for global..already using disableAll in CreateAITelemetryHandle..added this to prevent conflict
cni/network/network.go
Outdated
| return k8sPodName, k8sNamespace, nil | ||
| } | ||
|
|
||
| func setCustomDimensions(cniMetric *telemetry.CNIReport, nwCfg *cni.NetworkConfig, err error) { |
There was a problem hiding this comment.
not sure if you need to check if the other pointers are nil here
There was a problem hiding this comment.
not needed..nothing can be nil..
| tb.ConnectToTelemetryService(telemetryNumRetries, telemetryWaitTimeInMilliseconds) | ||
| defer tb.Close() | ||
|
|
||
| netPlugin.SetCNIReport(cniReport, tb) |
There was a problem hiding this comment.
tb := telemetry.NewTelemetryBuffer("")
| // TelemetryConfig - telemetry config read by telemetry service | ||
| type TelemetryConfig struct { | ||
| ReportToHostIntervalInSeconds time.Duration `json:"reportToHostIntervalInSeconds"` | ||
| DisableAll bool |
There was a problem hiding this comment.
where is this getting used?
There was a problem hiding this comment.
used by telemetry service
telemetry/telemetry.go
Outdated
| return report, err | ||
| } | ||
|
|
||
| func SendCNIMetric(cniReport *CNIReport, tb *TelemetryBuffer) error { |
There was a problem hiding this comment.
instead of creating different functions for CNS, such as SendCNSMetric, can this be just one function like SendMetric and takes the type / *Report.
Can it take *Report to reuse the func signature between CNI, CNS ?
There was a problem hiding this comment.
nope. CNI uses telemetry service but not cns
saiyan86
changed the title
jaer-tsun
changed the title
aitelemetry/telemetrywrapper.go
Outdated
| id string, | ||
| aiConfig AIConfig, | ||
| ) TelemetryHandle { | ||
| env, _ := os.LookupEnv("AZACN_TESTENV") |
There was a problem hiding this comment.
is it supposed to say AZACN_CLOUDENV ? or just ENV ?
There was a problem hiding this comment.
this is for uni test..just for making sure not conflicting with any other env variable
There was a problem hiding this comment.
No what I mean is why does the variable name says TESTENV. I thought it's a test environment - which is not the case here. I think it can be a bit misleading
There was a problem hiding this comment.
this is for unit test to pass...unit test can't make call to metadata server..let me check if i can do with mockagent by passing url
| enableSnatForDns bool | ||
| nwDNSInfo network.DNSInfo | ||
| cniMetric telemetry.CNIReport | ||
| cniMetric telemetry.AIMetric |
There was a problem hiding this comment.
i dont want to change existing CNIReport structure as this schema used by hostnetagent
|
|
||
| var payloadSize uint16 = 0 | ||
| var ( | ||
| payloadSize uint16 = 0 |
There was a problem hiding this comment.
I don;t think you need to set this to 0. GO inits variables ( like bool below ). You can choose to keep it / remove it.
There was a problem hiding this comment.
i didnt add this..i just moved this
jaer-tsun
changed the title
…working into cni_aitelemetry
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
* Added AITelemetry support for CNI * added new files * added other configs in config file * fixed ut * updated disableall similar to cns * added container name to report * addressed review comments * addressed review comments * added check for azure environment * added log * close log handle in unit test * addressed review comments * addressed review comments * fixed a condition * keep the netagent channel for logs * fixed error * addressed review comments
3 participants
What this PR does / why we need it:
This PR adds AI telemetry support for CNI via telemetry service. The CNI will send telemetry data to telemetry service and telemetry service in turn will send data to AppInsight.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Release note: