Migrate updates to Prometheus receiver from opencensus-service #208

dinooliva · 2019-07-30T22:05:39Z

No description provided.

codecov-io · 2019-07-30T22:21:07Z

Codecov Report

Merging #208 into master will increase coverage by 0.91%.
The diff coverage is 90.43%.

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
+ Coverage   72.74%   73.65%   +0.91%     
==========================================
  Files         107      112       +5     
  Lines        6108     6560     +452     
==========================================
+ Hits         4443     4832     +389     
- Misses       1438     1484      +46     
- Partials      227      244      +17

Impacted Files	Coverage Δ
receiver/opencensusreceiver/factory.go	`88.57% <ø> (ø)`	⬆️
exporter/exporterhelper/common.go	`100% <ø> (ø)`	⬆️
...r/addattributesprocessor/addattributesprocessor.go	`100% <ø> (ø)`	⬆️
processor/tailsampling/metrics.go	`0% <ø> (ø)`
internal/collector/processor/metrics.go	`0% <ø> (ø)`	⬆️
receiver/vmmetricsreceiver/metrics_receiver.go	`0% <ø> (ø)`	⬆️
config/configmodels/configmodels.go	`0% <ø> (ø)`	⬆️
compression/grpc/grpc.go	`100% <ø> (ø)`
receiver/prometheusreceiver/internal/metadata.go	`0% <ø> (-75%)`	⬇️
service/telemetry.go	`72.97% <ø> (ø)`	⬆️
... and 53 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 e87131c...4389c22. Read the comment docs.

songy23

LGTM

To other reviewers: FYI this is mostly a cherry-pick of census-instrumentation/opencensus-service#597 and census-instrumentation/opencensus-service#613 (already reviewed).

Remove stale content, put in place a simple skeleton, and put information for the Zipkin exporter.

ccaraman

Thank you for updating the documentation!

ccaraman · 2019-07-30T22:37:08Z

receiver/prometheusreceiver/README.md


 ScrapeLoop strips out any tag with empty value, however, in OpenCensus, the tag keys is stored separately, we need to able to get all the possible tag keys
 of the same metric family before committing the metric family to the sink.

+5. StartTimestamp and values of metrics of cumulative types
+
+In OpenCensus, every metrics of cumulative type is required to have a StartTimestamp, which records when a metric is first recorded, however, Prometheus


nit: OpenTelemetry instead of OpenCensus. :)

Would it be possible to replace all mentions of OpenCensus with OpenTelemetry?

Done - also change references to ocagent to otelsvc.

ccaraman · 2019-07-30T22:42:18Z

receiver/prometheusreceiver/README.md

+http_requests_total{method="post",code="200"} 1028
+http_requests_total{method="post",code="400"}    5
+```
+


Separate sentence for how the first scrape is used.

ccaraman · 2019-07-30T22:43:00Z

receiver/prometheusreceiver/README.md

@@ -232,16 +239,27 @@ Counter as described in the [Prometheus Metric Types Document](https://prometheu
 > is a cumulative metric that represents a single monotonically increasing counter whose value can only increase or be 
 > reset to zero on restart

-It is one of the most simple metric types we can be found in both systems. Examples of Prometheus Counters is as shown 
-below:
+It is one of the most simple metric types we can find in both systems. However, it is a cumulative type of metric, 


nit: 'we can find' -> 'found' ie simple metrics types found in both systems.

ccaraman · 2019-07-30T22:43:36Z

receiver/prometheusreceiver/README.md

-It is one of the most simple metric types we can be found in both systems. Examples of Prometheus Counters is as shown 
-below:
+It is one of the most simple metric types we can find in both systems. However, it is a cumulative type of metric, 
+considering with have two continuous scrapes from a target, with the first one as shown below:


Can you reword 'considering with have two continuous scrapes'? It is a bit hard to follow.

ccaraman · 2019-07-30T22:44:11Z

receiver/prometheusreceiver/README.md

@@ -403,12 +441,19 @@ Prometheus. We have to set this value to `0` instead.

 ### Gaugehistogram

-This is an undocumented data type, it shall be same as regular [Histogram](#histogram)
+This is an undocumented data type, and it's not supported currently


nit: full stop at the end of the sentence.

ccaraman · 2019-07-30T22:45:07Z

receiver/prometheusreceiver/README.md

 provided in Prometheus, and `nil` will be used for these values.

+Other than that, in some prometheus implementations, such as the Python version, Summary is allowed to have no quantiles, in a case


nit: full stop at the end of the sentence.

* Added Jaeger gRPC receiver Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Addressed first review round Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Changes based on the feedback from the second review Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Fixed import order, check for same ip:port on endpoints Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de> * Reverted the port-check Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

Signed-off-by: Hui Kang <kangh@us.ibm.com>

Refactored oc exporter code to make it easier to import in derived builds without copying all the code. Example derived exporter: https://github.com/owais/example-derived-oc-exporter - Moved internal/compression to root - Split opencensusexporter factory's `CreateTraceExporter` method into - `OCAgentOptions` and `CreateOCAgent`

MD5 is insecure cryptographic hash functions, and are therefore unsuitable for security applications.

* Add goimports check and fix import order for all files Updates #155. * Try specifying a version for goimports * Show the diff instead of files and fix import * Fix default in Makefile

* Add factory and update config for tail sampling processor Updates #146 * Move to package processor * Remove an unnecessary check * Move CreateDefaultConfig to factory and add unit tests * Fix test failure * Remove commented code

* Add misspell check and fix all typos Updates #155. * Format * Include yaml files

pjanotti

I read the text but very little of the code, I know that there were already a detailed code review on OCSvc repo. Just for sanity I will finish a quick pass tomorrow.

go.mod

receiver/prometheusreceiver/README.md

receiver/prometheusreceiver/internal/metricfamily.go

songy23

Please rebase and resolve the merge conflicts.

…#217) Follow up of #200. Second step of #146.

* Add Observability Vision document This is a guidance document that developers can consult with when working on improving own observability of OpenTelemetry Service. * PR fixes

* Add Zipkin exporter factory Add the Zipkin exporter configuration factory using the new configuration format. This does not bring many of the settings from the old configuration since they won't be used. In a sub-sequent PR the code of the exporter itself will be changed. * PR Feedback: add 1 test plus some comments * Rename endpoint to http-url and related field * Fix goimports issue * Remove TODO commented code Replaced the TODO commented code with an issue. * Rename the field used to specify destination

* First round of receiver and opencensus receiver documentation. * Undo go mod/sum changes * Address initial set of comments. * Address next set of comments. * Address next set of comments. * Fix use of server instead of receiver in comment and explain settings can be mix and matched. * Merged master and fixed mispell error caugh with new tools

* Add static check Fixes #155. * Fix most staticcheck errors * More fixes * Fix id_batcher

The paramater was named exportFormat but it actually used as the exporter name.

* Add Jaeger gRPC exporter Adds a Jaeger gRPC exporter. * Update exporters/README.md * Fixes on the exporter/README.md * Add doc.go with package description. * Fix import order on config_test.go

pjanotti

Hit @dinooliva - I did a quick pass and seems good, could you please rebase so we can be sure that all is well?

dinooliva · 2019-08-06T02:07:22Z

Thanks @pjanotti - I've rebased and fixed issues from the new static checks, should be good to go.

pjanotti

LGTM.

dinooliva · 2019-08-07T05:13:40Z

Thanks! When do you think this can be merged?

pjanotti · 2019-08-07T14:05:46Z

Merged it is, thanks @dinooliva.

* Add Linux support bundle script - Ensure executable - Update troubleshooting guide with support case details - Move upstream troubleshooting from README.md to troubleshooting.md - Add tests to existing installer_test * Attempt to fix execution bit * Address feedback * Update docs/troubleshooting.md Co-authored-by: Ryan Fitzpatrick <rmfitzpatrick@users.noreply.github.com> * Update docs/troubleshooting.md Co-authored-by: Ryan Fitzpatrick <rmfitzpatrick@users.noreply.github.com> * Update docs/troubleshooting.md Co-authored-by: Ryan Fitzpatrick <rmfitzpatrick@users.noreply.github.com>

dinooliva added 2 commits July 30, 2019 13:08

Migrate updates to Prometheus receiver from opencensus-service

4b1f6b1

Remove unnecessary configuration for adjusting metrics

58f046d

dinooliva requested review from bogdandrutu, flands, pjanotti, songy23 and tigrannajaryan as code owners July 30, 2019 22:05

songy23 approved these changes Jul 30, 2019

View reviewed changes

songy23 assigned dinooliva Jul 30, 2019

Update exporters README (#210)

26593b3

Remove stale content, put in place a simple skeleton, and put information for the Zipkin exporter.

ccaraman reviewed Jul 31, 2019

View reviewed changes

jpkrohling and others added 7 commits July 31, 2019 09:52

Fix README - remove broken links and commands (#211)

9da5d05

Signed-off-by: Hui Kang <kangh@us.ibm.com>

Use sha256 instead of md5 in node batcher processor (#202)

bcc9a41

MD5 is insecure cryptographic hash functions, and are therefore unsuitable for security applications.

Add goimports check and fix import order for all files (#209)

c7eacad

* Add goimports check and fix import order for all files Updates #155. * Try specifying a version for goimports * Show the diff instead of files and fix import * Fix default in Makefile

Add misspell check and fix all typos (#214)

dc4266d

* Add misspell check and fix all typos Updates #155. * Format * Include yaml files

pjanotti reviewed Aug 1, 2019

View reviewed changes

songy23 reviewed Aug 1, 2019

View reviewed changes

songy23 and others added 9 commits August 1, 2019 11:06

Move tail sampling config to its own package and remove stale configs (…

d0b9b47

…#217) Follow up of #200. Second step of #146.

Add Observability Vision document (#188)

3a985da

* Add Observability Vision document This is a guidance document that developers can consult with when working on improving own observability of OpenTelemetry Service. * PR fixes

Update README to address reviewer comments.

2a0f820

Update module dependencies and import order for metricfamily

437c680

Add static check and fix all errors (#218)

79957ff

* Add static check Fixes #155. * Fix most staticcheck errors * More fixes * Fix id_batcher

Rename exporterhelp parameter (#220)

3af9c48

The paramater was named exportFormat but it actually used as the exporter name.

Fix build: lower casing error message (#224)

fdb5047

Add jaeger grpc exporter (#219)

b39842b

* Add Jaeger gRPC exporter Adds a Jaeger gRPC exporter. * Update exporters/README.md * Fixes on the exporter/README.md * Add doc.go with package description. * Fix import order on config_test.go

pjanotti reviewed Aug 5, 2019

View reviewed changes

dinooliva added 7 commits August 5, 2019 17:51

Migrate updates to Prometheus receiver from opencensus-service

b1d106f

Remove unnecessary configuration for adjusting metrics

0e5a037

Update README to address reviewer comments.

0237bb0

Update module dependencies and import order for metricfamily

76bd726

Resolve merge conflicts

99d3bfa

Fix goimports

14d3201

Fix staticcheck issues

4389c22

songy23 approved these changes Aug 6, 2019

View reviewed changes

pjanotti approved these changes Aug 7, 2019

View reviewed changes

pjanotti merged commit 22b9a57 into open-telemetry:master Aug 7, 2019

dinooliva deleted the prometheus-receiver-migration branch August 7, 2019 23:00

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

Remove default parameters from Recordable interface (open-telemetry#208)

b096d8e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate updates to Prometheus receiver from opencensus-service #208

Migrate updates to Prometheus receiver from opencensus-service #208

dinooliva commented Jul 30, 2019

codecov-io commented Jul 30, 2019 •

edited

Loading

songy23 left a comment

ccaraman left a comment

ccaraman Jul 30, 2019

ccaraman Jul 30, 2019

dinooliva Aug 1, 2019

ccaraman Jul 30, 2019

dinooliva Aug 1, 2019

ccaraman Jul 30, 2019

dinooliva Aug 1, 2019

ccaraman Jul 30, 2019

dinooliva Aug 1, 2019

ccaraman Jul 30, 2019

dinooliva Aug 1, 2019

ccaraman Jul 30, 2019

dinooliva Aug 1, 2019

pjanotti left a comment

songy23 left a comment

pjanotti left a comment

dinooliva commented Aug 6, 2019

pjanotti left a comment

dinooliva commented Aug 7, 2019

pjanotti commented Aug 7, 2019

		provided in Prometheus, and `nil` will be used for these values.

		Other than that, in some prometheus implementations, such as the Python version, Summary is allowed to have no quantiles, in a case

Migrate updates to Prometheus receiver from opencensus-service #208

Migrate updates to Prometheus receiver from opencensus-service #208

Conversation

dinooliva commented Jul 30, 2019

codecov-io commented Jul 30, 2019 • edited Loading

Codecov Report

songy23 left a comment

Choose a reason for hiding this comment

ccaraman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

songy23 left a comment

Choose a reason for hiding this comment

pjanotti left a comment

Choose a reason for hiding this comment

dinooliva commented Aug 6, 2019

pjanotti left a comment

Choose a reason for hiding this comment

dinooliva commented Aug 7, 2019

pjanotti commented Aug 7, 2019

codecov-io commented Jul 30, 2019 •

edited

Loading