Skip to content

[signalfxexporter] Split incoming data requests by access token before enqueuing #1727

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

Merged
merged 6 commits into from
Nov 30, 2020

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Nov 30, 2020

Fixes a possible memory corruption caused by the fact that the previous logic was deleting an element from the resource attributes, and the data could be shared between different exporters (exporters are not allowed to manipulate data).

Updates #1495

Signed-off-by: Bogdan Drutu [email protected]

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Do we need similar changes for sapmexporter?

@bogdandrutu
Copy link
Member Author

Yes, they will be next

Signed-off-by: Bogdan Drutu <[email protected]>
metricToken := s.retrieveAccessToken(rms.At(0))

var sfxDataPoints []*sfxpb.DataPoint

for i := 0; i < rms.Len(); i++ {
rm := rms.At(i)
if rm.IsNil() {
continue
}
Copy link
Member

Choose a reason for hiding this comment

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

If we had debug asserts (if Go had them?) it would be useful to assert that s.retrieveAccessToken(rm)==metricToken. Maybe we need to add our own debug assert function (if it's even doable nicely in Go).
Nothing to do for now, but some food for thought.

Signed-off-by: Bogdan Drutu <[email protected]>
Signed-off-by: Bogdan Drutu <[email protected]>
@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #1727 (871b721) into master (225aa38) will increase coverage by 0.00%.
The diff coverage is 70.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1727   +/-   ##
=======================================
  Coverage   89.55%   89.56%           
=======================================
  Files         373      374    +1     
  Lines       18234    18296   +62     
=======================================
+ Hits        16330    16386   +56     
- Misses       1414     1415    +1     
- Partials      490      495    +5     
Flag Coverage Δ
integration 70.97% <ø> (ø)
unit 88.26% <70.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
exporter/signalfxexporter/translation/converter.go 90.49% <0.00%> (-0.83%) ⬇️
exporter/signalfxexporter/eventclient.go 81.48% <69.23%> (-4.73%) ⬇️
exporter/signalfxexporter/dpclient.go 82.60% <71.42%> (-1.40%) ⬇️
exporter/signalfxexporter/factory.go 85.00% <73.33%> (-3.06%) ⬇️
exporter/signalfxexporter/exporter.go 97.14% <100.00%> (+2.40%) ⬆️
pkg/batchperresourceattr/batchperresourceattr.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 225aa38...871b721. Read the comment docs.

@bogdandrutu bogdandrutu merged commit 80dddc3 into open-telemetry:master Nov 30, 2020
@bogdandrutu bogdandrutu deleted the signalfxsplit branch November 30, 2020 21:48
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
* Update SamplingParameters

Remove HasRemoteParent fields from SamplingParameters. The
HasRemoteParent field is a duplicate of the Remote field of the parent
span context contained in the ParentContext.

Change the `ParentContext` field from storing a `SpanContext` to a
`context.Context` that holds the parent span. This is to conform with
the OpenTelemetry specification and resolve #1727.

* Update PR number
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