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.

Sign up for GitHub

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

Jump to bottom

fix!(sumologicexporter): send resource attributes as fields for non-otlp, removing metadata_attributes #549

Merged
merged 3 commits into from
Apr 25, 2022
Merged

fix!(sumologicexporter): send resource attributes as fields for non-otlp, removing metadata_attributes #549

merged 3 commits into from
Apr 25, 2022

Conversation

pmalek-sumo
Copy link
Contributor

No description provided.

@pmalek-sumo pmalek-sumo self-assigned this Apr 14, 2022
@github-actions github-actions bot added the go label Apr 14, 2022
@pmalek-sumo pmalek-sumo force-pushed the fix-exporter-batching branch 2 times, most recently from 5228e9f to 82925a3 Compare April 19, 2022 13:45
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 19, 2022
@pmalek-sumo pmalek-sumo changed the title fix!(sumologicexporter): send resource attributes as fields, deprecating metadata_attributes for text and json logs fix!(sumologicexporter): send resource attributes as fields, deprecating metadata_attributes for non-otlp Apr 19, 2022
@pmalek-sumo pmalek-sumo changed the title fix!(sumologicexporter): send resource attributes as fields, deprecating metadata_attributes for non-otlp fix!(sumologicexporter): send resource attributes as fields, removing metadata_attributes Apr 19, 2022
@pmalek-sumo pmalek-sumo force-pushed the fix-exporter-batching branch 2 times, most recently from 53c119d to 2c97487 Compare April 19, 2022 13:54
@pmalek-sumo pmalek-sumo changed the title fix!(sumologicexporter): send resource attributes as fields, removing metadata_attributes fix!(sumologicexporter): send resource attributes as fields for non-otlp, removing metadata_attributes Apr 19, 2022
@pmalek-sumo pmalek-sumo force-pushed the fix-exporter-batching branch from 2c97487 to 726bdaa Compare April 19, 2022 14:08
@pmalek-sumo pmalek-sumo marked this pull request as ready for review April 19, 2022 14:09
@pmalek-sumo pmalek-sumo requested a review from a team as a code owner April 19, 2022 14:09
sumo-drosiek
sumo-drosiek reviewed Apr 20, 2022
View reviewed changes
pkg/exporter/sumologicexporter/exporter_test.go
Comment on lines +633 to +635
assert.Equal(t, "host.name=harry-potter, host.type=wizard", req.Header.Get("X-Sumo-Fields"), "X-Sumo-Fields")
assert.Equal(t, "harry-potter", req.Header.Get("X-Sumo-Category"), "X-Sumo-Category")
assert.Equal(t, "undefined", req.Header.Get("X-Sumo-Host"), "X-Sumo-Host")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we need the third argument, if yes, we should use it for all comparisons in tests 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of this is that we'll get this third argument in test failure output without the need to look at the code/line number.

I agree but at the same time I didn't want to change ALL TEST THE CODE. Perhaps an improvement for another time?

Copy link
Contributor

Choose a reason for hiding this comment

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

The point of this is that we'll get this third argument in test failure output without the need to look at the code/line number.

Most of times I need to do it anyway 😅

sumo-drosiek
sumo-drosiek reviewed Apr 20, 2022
View reviewed changes
pkg/exporter/sumologicexporter/sender.go
errs []error
droppedRecords []metricPair
currentRecords []metricPair
droppedRecords []pdata.Metric
Copy link
Contributor

Choose a reason for hiding this comment

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

what about resource attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're sending metrics from a particular resource (see the argument to this func: pdata.ResourceMetrics) so no need to set its attributes aside because we'll have it readily available at the call site.

sumologic-otel-collector/pkg/exporter/sumologicexporter/exporter.go

Lines 334 to 337 in 726bdaa

dropped = append(dropped, droppedResourceMetrics{
resource: rm.Resource(),
metrics: droppedMetrics,
})

sumo-drosiek
sumo-drosiek reviewed Apr 20, 2022
View reviewed changes
Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

I don't see a code where we translate record attributes. Do we decided to skip translation for them?

@sumo-drosiek
Copy link
Contributor

Do we decided to skip translation for them?

Yes, we decided to skip translation for them

swiatekm
swiatekm reviewed Apr 21, 2022
View reviewed changes
Copy link

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

The changes look ok to me. I'm just wandering if we should add documentation regarding the breaking change in this PR (in the changelog?) or leave it for the release notes? I'd rather do it here to be honest.

@swiatekm swiatekm force-pushed the fix-exporter-batching branch 7 times, most recently from 1ce53ab to 9a6919d Compare April 25, 2022 10:26
@swiatekm swiatekm force-pushed the fix-exporter-batching branch from 9a6919d to 20f6ebf Compare April 25, 2022 11:05
@swiatekm swiatekm force-pushed the fix-exporter-batching branch 2 times, most recently from 4f87878 to fbab92c Compare April 25, 2022 12:13
sumo-drosiek
sumo-drosiek reviewed Apr 25, 2022
View reviewed changes
docs/Upgrading.md Outdated Show resolved Hide resolved
@swiatekm swiatekm requested a review from sumo-drosiek April 25, 2022 12:48
@swiatekm swiatekm force-pushed the fix-exporter-batching branch from 8c2d22c to 013aff7 Compare April 25, 2022 13:08
sumo-drosiek
sumo-drosiek approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@sumo-drosiek sumo-drosiek left a comment

Choose a reason for hiding this comment

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

Let's ship it

@swiatekm swiatekm merged commit d44bde0 into main Apr 25, 2022
@swiatekm swiatekm deleted the fix-exporter-batching branch April 25, 2022 13:55
@swiatekm swiatekm mentioned this pull request Apr 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swiatekm swiatekm swiatekm left review comments

@sumo-drosiek sumo-drosiek sumo-drosiek approved these changes

Assignees

@pmalek-sumo pmalek-sumo

Labels
documentation Improvements or additions to documentation go
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pmalek-sumo @sumo-drosiek @swiatekm @pmalek