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.

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

[sumologicexporter] Add sender #1693

Merged

Conversation

sumo-drosiek
Copy link
Member

Description:

Add sender to Sumo Logic exporter as part of #1498
Sender is responsible for building request body and to send it to the service with addition of required headers.
It avoids sending too big request by batching the body

Link to tracking Issue: #1498

Testing:

  • unit tests

Documentation:

  • comments in code

@codecov
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #1693 (be15195) into master (eb1946b) will increase coverage by 0.02%.
The diff coverage is 90.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1693      +/-   ##
==========================================
+ Coverage   89.39%   89.41%   +0.02%     
==========================================
  Files         371      372       +1     
  Lines       18166    18269     +103     
==========================================
+ Hits        16240    16336      +96     
- Misses       1434     1438       +4     
- Partials      492      495       +3     
Flag Coverage Δ
integration 71.04% <ø> (ø)
unit 88.12% <90.29%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
exporter/sumologicexporter/sender.go 90.29% <90.29%> (ø)
processor/groupbytraceprocessor/event.go 96.77% <0.00%> (+0.80%) ⬆️
receiver/k8sclusterreceiver/watcher.go 97.64% <0.00%> (+2.35%) ⬆️

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 eb1946b...be15195. Read the comment docs.

@bogdandrutu
Copy link
Member

@sumo-drosiek can you please try to make codecov happy?

exporter/sumologicexporter/sender.go Outdated Show resolved Hide resolved
exporter/sumologicexporter/sender.go Outdated Show resolved Hide resolved
exporter/sumologicexporter/sender.go Outdated Show resolved Hide resolved
exporter/sumologicexporter/sender.go Outdated Show resolved Hide resolved
exporter/sumologicexporter/sender.go Outdated Show resolved Hide resolved
exporter/sumologicexporter/sender.go Outdated Show resolved Hide resolved
exporter/sumologicexporter/sender.go Outdated Show resolved Hide resolved
exporter/sumologicexporter/sender.go Show resolved Hide resolved
@sumo-drosiek
Copy link
Member Author

sumo-drosiek commented Nov 26, 2020

@bogdandrutu I tried to make codecov happy, but I don't have idea how to cover body.WriteString and json.Marshal errors 🤷‍♂️

exporter/sumologicexporter/sender.go Show resolved Hide resolved
droppedRecords []pdata.LogRecord
currentRecords []pdata.LogRecord
formattedLine string
err error
Copy link
Member

Choose a reason for hiding this comment

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

The same comment applies to other properties, but do you need err to be at this scope? You are one bug away from a problem that is hard to detect :-) Move the err inside the loop, so that you won't leak it elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved together with formattedLine. Other variables require broaden scope

exporter/sumologicexporter/sender.go Show resolved Hide resolved
exporter/sumologicexporter/sender.go Show resolved Hide resolved
exporter/sumologicexporter/sender.go Show resolved Hide resolved
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

There are still a few places where the coverage is missing, especially on error handling, but LGTM overall.

exporter/sumologicexporter/sender.go Show resolved Hide resolved
@bogdandrutu bogdandrutu merged commit 941b403 into open-telemetry:master Dec 1, 2020
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
This patch removes `ApplyConfig` method and `Config` struct from
`go.opentelemetry.io/otel/sdk/trace` package.  To ensure valid config
for TracerProvider, it adds `ensureValidTracerProviderConfig` private
function.
Jaeger and Zipkin have been used the `Config` directly across package
boundaries. Since `Config` is removed, they can't use it. This change,
thus, replaces `WithSDK` with `WithSDKOptions`.

Resolves #1636, #1705.
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