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

Add trace exports to Splunk HEC #323

Closed
wants to merge 5 commits into from
Closed

Add trace exports to Splunk HEC #323

wants to merge 5 commits into from

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Jun 15, 2020

Description:
Adds trace exports support to Splunk HEC.

Testing:
Unit tests.

Documentation:
No doc - this is complementary to the metric export support with no additional config keys.

@atoulme atoulme requested a review from a team June 15, 2020 00:02
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 15, 2020 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

  • ✅ Antoine Toulme (94010efac548a9a719b2db43c745d183e5c39e98)

@atoulme atoulme force-pushed the export_spans_to_hec branch 4 times, most recently from cf85bba to 3373ef3 Compare June 15, 2020 01:03
@atoulme atoulme force-pushed the export_spans_to_hec branch from 3373ef3 to 38dcf4b Compare June 15, 2020 01:44
@flands flands added this to the 0.5.0 milestone Jun 17, 2020
@flands
Copy link
Contributor

flands commented Jun 23, 2020

@atoulme what is the latest on this?

@atoulme
Copy link
Contributor Author

atoulme commented Jun 23, 2020

We had a first review, and I addressed it. It's ready for another review or merge.

@rmfitzpatrick
Copy link
Contributor

We had a first review, and I addressed it. It's ready for another review or merge.

I'm under the impression the diff coverage target is a requirement for acceptance so this will need additional testing. Correct @tigrannajaryan?

rmfitzpatrick
rmfitzpatrick reviewed Jun 23, 2020
View reviewed changes
exporter/splunkhecexporter/tracedata_to_splunk.go
Source: config.Source,
SourceType: config.SourceType,
Index: config.Index,
Event: span,
Copy link
Contributor

Choose a reason for hiding this comment

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

What assurances are there that this won't shift with unrelated internal type changes? Seems less safe than the current metric translator:

opentelemetry-collector-contrib/exporter/splunkhecexporter/metricdata_to_splunk.go

Line 77 in 82b1adf

for _, tsPoint := range timeSeries.Points {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any assurances, and I have superficial knowledge of HEC. I read the docs, used it, and it works reasonably well for me.

I would however point out that Splunk is in GA using this format, and the format has been used since the inception of HEC. If anything I would expect Splunk to change its HEC input in a backwards-compatible manner and with long deprecation periods.

What assurances do you require? How can I help?

Copy link
Contributor

@rmfitzpatrick rmfitzpatrick Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sry, I'm referring specifically to the json encoding of the tracepb.Span* as is and not the overall HEC event format, which I have not worked with before. It's unclear to me that the spans are being marshaled as expected without using jsonpb/protojson directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I'll add tests for that. Maybe that will help coverage enough too :)

@tigrannajaryan
Copy link
Member

I'm under the impression the diff coverage target is a requirement for acceptance so this will need additional testing. Correct @tigrannajaryan?

Yes, it would be nice if we could get a bit better coverage.

@atoulme
Copy link
Contributor Author

atoulme commented Jun 23, 2020

Ask and you shall receive - we have test, and better coverage 🎉

@rmfitzpatrick
Copy link
Contributor

Looks like the coverage checks weren't triggered with the latest push. @atoulme maybe try pushing an amended commit? LGTM otherwise (assuming the b64 ids are desired).

@atoulme atoulme force-pushed the export_spans_to_hec branch from c80509f to 52098dd Compare June 24, 2020 16:50
@jrcamp
Copy link
Contributor

jrcamp commented Jun 24, 2020

@atoulme try merging in master to pick up some changes that were made to codecov

@jrcamp jrcamp closed this Jun 24, 2020
@jrcamp jrcamp reopened this Jun 24, 2020
@open-telemetry open-telemetry deleted a comment from codecov bot Jun 24, 2020
@jrcamp
Copy link
Contributor

jrcamp commented Jun 24, 2020

I'm not sure why :/ It's working in other PRs. Can you try opening a new PR if it's not too much hassle and closing this one?

@atoulme
Copy link
Contributor Author

atoulme commented Jun 24, 2020

Trying that.

@atoulme atoulme closed this Jun 24, 2020
@atoulme atoulme mentioned this pull request Jun 24, 2020
Merged
mxiamxia referenced this pull request in mxiamxia/opentelemetry-collector-contrib Jul 22, 2020
@ccaraman @tigrannajaryan
Expose public method to create OC Exporter (#323)
2779b99 
Refactored the code to make NewMetricsExporter/NewTraceExporter public.
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
@ferhatelmas @rghetia
Fix url to website in readme (#323)
90ba74e
bogdandrutu pushed a commit that referenced this pull request May 12, 2022
@cpheps
CSV Idiomatic Errors (#323)
fa36116 
* Changed errors in csv operator to have lowercase first word to be idiomatic

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>

* Removed fmt.Errorf in csv for errors that did not require formatting

Signed-off-by: Corbin Phelps <corbin.phelps@bluemedora.com>
codeboten pushed a commit that referenced this pull request Nov 23, 2022
@ocelotl @c24t
Add posargs (#324)
0f88a05 
Fixes #323
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmfitzpatrick rmfitzpatrick rmfitzpatrick approved these changes

Assignees

@rmfitzpatrick rmfitzpatrick

Labels
None yet
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.
5 participants
@atoulme @flands @rmfitzpatrick @tigrannajaryan @jrcamp