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

Make pdata.LogsToOtlp and pdata.LogsFromOtlp impossible to call from outside this module #1703

Conversation

tigrannajaryan
Copy link
Member

@tigrannajaryan tigrannajaryan commented Sep 1, 2020

LogsToOtlp and LogsFromOtlp are intended to be used only by OTLP exporter (for which
OTLP is the native data format) and fileexporter (for which we do want to output internal
representation for debugging purposes).

All other code is supposed to use public member functions of pdata.Logs struct.

This changes makes it impossible to use LogsToOtlp and LogsFromOtlp outside this module
by hiding them behind an intermediary struct that is declared in an internal package.

This is necessary to prevent accidental use of LogsToOtlp and LogsFromOtlp and the OTLP
data structs, which we prohibit to do since the OTLP data structs may change in the
future.

@project-bot project-bot bot added this to In progress in Collector Sep 1, 2020
@tigrannajaryan tigrannajaryan force-pushed the feature/tigran/hide-otlp-funcs branch 2 times, most recently from 195006d to a4694aa Compare September 1, 2020 17:15
@codecov
Copy link

codecov bot commented Sep 1, 2020

Codecov Report

Merging #1703 into master will increase coverage by 0.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1703      +/-   ##
==========================================
+ Coverage   92.17%   92.31%   +0.14%     
==========================================
  Files         259      259              
  Lines       18402    18211     -191     
==========================================
- Hits        16962    16812     -150     
+ Misses       1013      987      -26     
+ Partials      427      412      -15     
Impacted Files Coverage Δ
consumer/pdata/log.go 100.00% <100.00%> (ø)
exporter/fileexporter/file_exporter.go 83.33% <100.00%> (ø)
exporter/otlpexporter/otlp.go 75.53% <100.00%> (+0.26%) ⬆️
internal/data/testdata/log.go 100.00% <100.00%> (ø)
internal/otlp_wrapper.go 100.00% <100.00%> (ø)
receiver/otlpreceiver/logs/otlp.go 100.00% <100.00%> (ø)
consumer/pdata/metric.go 87.73% <0.00%> (-10.36%) ⬇️
translator/internaldata/resource_to_oc.go 88.88% <0.00%> (-1.86%) ⬇️
processor/batchprocessor/batch_processor.go 98.14% <0.00%> (-0.35%) ⬇️
... and 43 more

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 c0b3c61...07db553. Read the comment docs.

tigrannajaryan pushed a commit to tigrannajaryan/opentelemetry-collector that referenced this pull request Sep 1, 2020
This is necessary to prevent circular package dependency that
can arise when we need to hide certain pdata functions in the
internal package in a commit that is coming soon in
open-telemetry#1703
tigrannajaryan added a commit that referenced this pull request Sep 1, 2020
This is necessary to prevent circular package dependency that
can arise when we need to hide certain pdata functions in the
internal package in a commit that is coming soon in
#1703
@tigrannajaryan tigrannajaryan changed the title [WIP] [DON'T REVIEW] Make pdata.LogsToOtlp impossible to call from outside this repo [WIP] [DON'T REVIEW] Make pdata.LogsToOtlp and pdata.LogsFromOtlp impossible to call from outside this module Sep 1, 2020
@tigrannajaryan tigrannajaryan changed the title [WIP] [DON'T REVIEW] Make pdata.LogsToOtlp and pdata.LogsFromOtlp impossible to call from outside this module Make pdata.LogsToOtlp and pdata.LogsFromOtlp impossible to call from outside this module Sep 1, 2020
bogdandrutu pushed a commit to bogdandrutu/opentelemetry-collector that referenced this pull request Sep 1, 2020
This is necessary to prevent circular package dependency that
can arise when we need to hide certain pdata functions in the
internal package in a commit that is coming soon in
open-telemetry#1703
bogdandrutu pushed a commit to bogdandrutu/opentelemetry-collector that referenced this pull request Sep 1, 2020
This is necessary to prevent circular package dependency that
can arise when we need to hide certain pdata functions in the
internal package in a commit that is coming soon in
open-telemetry#1703
bogdandrutu added a commit that referenced this pull request Sep 1, 2020
This is necessary to prevent circular package dependency that
can arise when we need to hide certain pdata functions in the
internal package in a commit that is coming soon in
#1703

Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
@tigrannajaryan
Copy link
Member Author

@open-telemetry/collector-approvers I need a review/approval on this.

…outside this module

LogsToOtlp and LogsFromOtlp are intended to be used only by OTLP exporter (for which
OTLP is the native data format) and fileexporter (for which we do want to output internal
representation for debugging purposes).

All other code is supposed to use public member functions of pdata.Logs struct.

This changes makes it impossible to use LogsToOtlp and LogsFromOtlp outside this module
by hiding them behind an intermediary struct that is declared in an internal package.

This is necessary to prevent accidental use of LogsToOtlp and LogsFromOtlp and the OTLP
data structs, which we prohibit to do since the OTLP data structs may change in the
future.
Collector automation moved this from In progress to Reviewer approved Sep 11, 2020
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

LGTM

@tigrannajaryan tigrannajaryan merged commit 075828f into open-telemetry:master Sep 14, 2020
Collector automation moved this from Reviewer approved to Done Sep 14, 2020
@tigrannajaryan tigrannajaryan deleted the feature/tigran/hide-otlp-funcs branch September 14, 2020 14:43
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this pull request Nov 11, 2021
* Add Valid method to KeyValue

* Use KeyValue.Valid in attribute add on Span

* Resource StringDetector errors for invalid attribute

* Ignore invalid attr in NewWithAttributes

The OpenTelemetry specification requires attributes conform to a
standard evaluated and returned by attribute.KeyValue.Valid. To comply
with the specification, Resources created from NewWithAttributes need to
only contain valid attributes. This adds a check to ensure this and
drops invalid attributes passed as arguments.

* Add changes to changelog

* Add nolint comment

The attribute.Set is (possibly overly) optimized to avoid allocations.
The returned value from the constructor is a value of a Set, not a
pointer to the Set. A Set contains a lock value and pointer methods so
passing the Set value raises the copylock go vet error. This copies the
same nolint comment from the `NewSet` method this used to use.

* Apply suggestions from code review

Co-authored-by: Sam Xie <xsambundy@gmail.com>

Co-authored-by: Sam Xie <xsambundy@gmail.com>
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Collector
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants