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

New Relic Exporter #229

Merged
merged 29 commits into from
Jun 3, 2020
Merged

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented May 12, 2020

Description: Adds a new exporter to send trace and metric data to New Relic.

Testing: Adds unit and integration tests.

Documentation: Includes functional comments and a README.md for general information on the exporter.

@MrAlias MrAlias requested a review from a team as a code owner May 12, 2020 00:07
exporter/newrelicexporter/newrelic.go Outdated Show resolved Hide resolved
exporter/newrelicexporter/README.md Outdated Show resolved Hide resolved
Copy link

@lykkin lykkin left a comment

Choose a reason for hiding this comment

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

couple of questions, LGTM though!

exporter/newrelicexporter/transformer.go Show resolved Hide resolved
exporter/newrelicexporter/transformer.go Show resolved Hide resolved
@bogdandrutu
Copy link
Member

Please rebase and fix imports :)

@MrAlias
Copy link
Contributor Author

MrAlias commented May 18, 2020

Please rebase and fix imports :)

Slowly working on this. Your ask to use the new components is requiring a complete redo of most of this code. I'm working on it, but it is slow going.

Collector automation moved this from Review in progress to Reviewer approved Jun 2, 2020
@bogdandrutu
Copy link
Member

Can you please rebase?

@MrAlias
Copy link
Contributor Author

MrAlias commented Jun 2, 2020

Can you please rebase?

I must be missing something, this is current with master currently.

@bogdandrutu did you mean another branch?

@MrAlias MrAlias requested a review from a team as a code owner June 2, 2020 21:11
@MrAlias
Copy link
Contributor Author

MrAlias commented Jun 2, 2020

Can you please rebase?

I must be missing something, this is current with master currently.

@bogdandrutu did you mean another branch?

Ah, my bad. I needed a refresh 🤦

@bogdandrutu
Copy link
Member

@MrAlias not sure why the loadtest fails always on this PR but not on master. I bet there is something going on.

exporter/newrelicexporter/go.mod Outdated Show resolved Hide resolved
exporter/newrelicexporter/go.mod Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor Author

MrAlias commented Jun 2, 2020

@MrAlias not sure why the loadtest fails always on this PR but not on master. I bet there is something going on.

I'm not sure why this is happening either, but I am able to "reproduce" locally. The failure is not consistent, different load tests seem to be failing on different runs. Not sure why yet.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jun 2, 2020

I'm also getting loadtest failures (when run locally) when run against master as well:

# Test Results
Started: Tue, 02 Jun 2020 16:11:54 -0700

Test                                    |Result|Duration|CPU Avg%|CPU Max%|RAM Avg MiB|RAM Max MiB|Sent Items|Received Items|
----------------------------------------|------|-------:|-------:|-------:|----------:|----------:|---------:|-------------:|
Metric10kDPS/OpenCensus                 |FAIL  |      3s|    65.3|    65.3|         44|         51|    217000|        207900|CPU consumption is 65.3%, max expected is 40%
Metric10kDPS/Carbon                     |PASS  |     16s|   149.9|   151.7|         42|         52|    721000|        721000|
Metric10kDPS/SignalFx                   |PASS  |     15s|    68.5|    71.3|         49|         60|    939400|        939400|
Trace10kSPS/OpenCensus                  |FAIL  |      3s|    54.6|    54.7|         30|         60|     31100|         27300|CPU consumption is 54.7%, max expected is 26%
Trace10kSPS/SAPM                        |FAIL  |      3s|    47.0|    47.0|         32|         64|     29600|         26700|CPU consumption is 47.0%, max expected is 24%

Maybe my previous reproductions of the failure were not accurate. Guessing the performance of the underlying hardware is contributing here.

@tigrannajaryan
Copy link
Member

I'm also getting loadtest failures (when run locally) when run against master as well:

# Test Results
Started: Tue, 02 Jun 2020 16:11:54 -0700

Test                                    |Result|Duration|CPU Avg%|CPU Max%|RAM Avg MiB|RAM Max MiB|Sent Items|Received Items|
----------------------------------------|------|-------:|-------:|-------:|----------:|----------:|---------:|-------------:|
Metric10kDPS/OpenCensus                 |FAIL  |      3s|    65.3|    65.3|         44|         51|    217000|        207900|CPU consumption is 65.3%, max expected is 40%
Metric10kDPS/Carbon                     |PASS  |     16s|   149.9|   151.7|         42|         52|    721000|        721000|
Metric10kDPS/SignalFx                   |PASS  |     15s|    68.5|    71.3|         49|         60|    939400|        939400|
Trace10kSPS/OpenCensus                  |FAIL  |      3s|    54.6|    54.7|         30|         60|     31100|         27300|CPU consumption is 54.7%, max expected is 26%
Trace10kSPS/SAPM                        |FAIL  |      3s|    47.0|    47.0|         32|         64|     29600|         26700|CPU consumption is 47.0%, max expected is 24%

Maybe my previous reproductions of the failure were not accurate. Guessing the performance of the underlying hardware is contributing here.

This is a performance regression caused by your go.mod file bringing new protobuf version:

github.com/golang/protobuf v1.4.1

We observed this in other PRs. I did not look into finding the root cause. Stay on v1.3.5 for now.

@bogdandrutu
Copy link
Member

@MrAlias I was wrong about which artifact was the issue, sorry. @tigrannajaryan is correct.

@MrAlias
Copy link
Contributor Author

MrAlias commented Jun 3, 2020

@bogdandrutu, @tigrannajaryan: looks like it was also the dependency on google.golang.org/protobuf causing the loadtest to fail: 5a8c663

The only code depending on this package was in the testing of Summary transforms. I commented it out (so I can still run it manually). I think keeping it this way until the move to the new pdata packages is undertaken seems like a reasonable option.

@bogdandrutu
Copy link
Member

@MrAlias so I was correct by "guessing" that dependency was wrong as well :))

@bogdandrutu bogdandrutu merged commit bef543e into open-telemetry:master Jun 3, 2020
Collector automation moved this from Reviewer approved to Done Jun 3, 2020
@MrAlias
Copy link
Contributor Author

MrAlias commented Jun 3, 2020

Thanks @bogdandrutu and @tigrannajaryan for all the help 😄

@MrAlias MrAlias deleted the newrelic-exporter branch June 3, 2020 17:41
wyTrivail referenced this pull request in mxiamxia/opentelemetry-collector-contrib Jul 13, 2020
* Initial tracing

* Config tests and refactor

* Factory tests

* Split transformer into own file

* Add integration test of exporting spans

* Add transform tests

* Initial metrics

* Add integration test for metric export

* Add transform tests

* Update version and include in project go.mod

* Add README

* Lint fixes

* Add to main components

* Fix lint

* Add units to default in README

* Remove sync.Pool

* Update collector package

* Fix lint

* Update go.mod

* Update top level go.mod and go.sum

Remove newrelicexporter version suffix and run `go mod tidy`.

* Clean go.sum for newrelicexporter

* Update go.mod

* Manually prune go.sum

* Try to fix loadtest

Remove dependency on google.golang.org/protobuf

* go mod tidy
mxiamxia referenced this pull request in mxiamxia/opentelemetry-collector-contrib Jul 22, 2020
* Refactor (Trace/Metrics)ExportFormat to ExporterName

Continuing the refactor that started from exporterhelper in proper naming the related fileds and methods.

* Remove Exporter interface and shorten method names
codeboten pushed a commit that referenced this pull request Nov 23, 2022
In particular, the following errors are fixed in this commit:

* Don't return False in __exit__

Returning a literal causes a mypy error when combined with the
`typing.Optional[bool]` type hint. Furthermore, exception handling is
the same when returning `False` and when returning `None` (the
exception is re-raised). Therefore, it's simpler to remove the return
statement and change the type hint to `None`.

* Correctly initialize nested tuple

Tuples of length 1 should be initialized with a trailing comma to be
properly interpreted.

* Pass correct type to use_context() in test

* Add type annotations for test helper functions

Since we have `disallow_untyped_calls = True` in our mypy config for
tests, we must add type annotations to any function that is called
from a test.


Addditionally, bump minimal mypy version to 0.740 to consistently reproduce these errors.
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