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

Remove the proto dependency in goldendataset for traces #3322

Merged
merged 16 commits into from
Jun 7, 2021

Conversation

IrisTuntun
Copy link
Contributor

Description:
Remove the proto dependency in goldendataset for generating traces. Functions being called by GenerateTraces() will use PData instead of protos for the final constructing of Traces.

Link to tracking Issue:
#3229

Testing: The unit test is updated to use PData in test cases.

@IrisTuntun IrisTuntun requested a review from a team May 26, 2021 21:58
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Please apply the comments everywhere they apply.

consumer/pdata/common.go Outdated Show resolved Hide resolved
internal/goldendataset/span_generator.go Outdated Show resolved Hide resolved
internal/goldendataset/traces_generator.go Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

https://github.com/open-telemetry/opentelemetry-collector/pull/3322/checks?check_run_id=2722314021 failing tests that use the golden data, so seems related to this.

@IrisTuntun
Copy link
Contributor Author

https://github.com/open-telemetry/opentelemetry-collector/pull/3322/checks?check_run_id=2722314021 failing tests that use the golden data, so seems related to this.

I think the root cause is that some functions like generateSpanLinks() used to return a nil pointer when the input is nil, but in my previous code, I have to return empty objects since the return type was not a pointer. This results in slightly different generated Spans and Traces than before. I changed the code to match the previous behavior.

@IrisTuntun IrisTuntun closed this Jun 3, 2021
@IrisTuntun IrisTuntun reopened this Jun 3, 2021
@IrisTuntun
Copy link
Contributor Author

I closed this PR by mistake. Then I reopened it and sorry for the inconvience.

@bogdandrutu bogdandrutu merged commit 8042a03 into open-telemetry:main Jun 7, 2021
parts.ArrayVal().Append(pdata.NewAttributeValueString("otelcol"))
parts.ArrayVal().Append(pdata.NewAttributeValueString("--config=/etc/otel-collector-config.yaml"))
parts.ArrayVal().Append(pdata.NewAttributeValueString("--mem-ballast-size-mib=683"))
attrMap["conventions.AttributeProcessCommandLine"] = parts
Copy link
Member

Choose a reason for hiding this comment

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

This is a bug, transformed from the constant conventions.AttributeProcessCommandLine to "conventions.AttributeProcessCommandLine". Fixed in #3377.

Comment on lines +89 to +91
if err != nil {
instrumentationLibrarySpansSlice.Append(*libSpans)
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a bug, fixed in #3377.

Copy link
Member

Choose a reason for hiding this comment

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

This bug causes actually to always produce empty data, so that is the reason the previous bug was hidden.

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