Improve OC Receiver Documentation #203

ccaraman · 2019-07-29T20:00:50Z

Please note this is a first pass. There will be a follow up pr to further document the oc receiver and clean up references to features/items that are no longer applicable to oc receiver.

Extend test yaml for opencensus receiver to show how to configure all values with comments on uses.
Add more notes to the structs on default values and how they are used within the code base.
Reorder receiver/README.md to list supported receivers in alphabetical order and add hyperlinks.
Add one line about what data type each receiver supports (metrics and/or traces)

Some clean ups that resulted from adding better documentation for OC Receiver

Begin update of configuration section in README.md
Add comments in several places where things need to be added to config/configmodels.go
- Diagnostics configuration is retrieved in service.go
Add validation/test to have at least one enabled receiver in configuration.
Add empty processor/README.md.

Note: There will be a follow up pr to complete documentation around open census receiver in receiver/readme.md in a different pr.

codecov-io · 2019-07-29T20:13:23Z

Codecov Report

Merging #203 into master will increase coverage by 0.04%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master     #203      +/-   ##
==========================================
+ Coverage   72.43%   72.47%   +0.04%     
==========================================
  Files         109      109              
  Lines        6305     6315      +10     
==========================================
+ Hits         4567     4577      +10     
  Misses       1508     1508              
  Partials      230      230

Impacted Files	Coverage Δ
receiver/opencensusreceiver/opencensus.go	`84.55% <ø> (ø)`	⬆️
service/service.go	`68.11% <ø> (ø)`	⬆️
receiver/opencensusreceiver/factory.go	`88.57% <ø> (ø)`	⬆️
config/configmodels/configmodels.go	`0% <ø> (ø)`	⬆️
internal/zpagesserver/zpagesserver.go	`88.23% <ø> (ø)`	⬆️
receiver/opencensusreceiver/config.go	`78.04% <0%> (ø)`	⬆️
config/config.go	`98.91% <100%> (+0.04%)`	⬆️

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 a1ecc6f...dbfbc7f. Read the comment docs.

README.md

config/config.go

receiver/README.md

README.md

config/configmodels/configmodels.go

receiver/README.md

receiver/opencensusreceiver/config.go

pjanotti

LGTM

tigrannajaryan · 2019-07-30T14:01:26Z

OC Receiver Documentation: First Pass

Nit: Use the imperative mood in the subject line (Rule 1 of https://chris.beams.io/posts/git-commit/, see https://github.com/open-telemetry/community/blob/master/CONTRIBUTING.md)

(I am assuming PR title is going to be the commit message)

tigrannajaryan

Mostly good changes.
I did one pass. The PR is somewhat large, so a bit difficult to review in details. I will do another round a bit later to see if I missed anything.

tigrannajaryan · 2019-07-30T14:02:48Z

README.md

@@ -121,14 +121,27 @@ exporting will resume.
 ## <a name="config"></a>Configuration

 The OpenTelemetry Service (both the Agent and Collector) is configured via a
-YAML file. In general, you need to configure one or more receivers as well as
-one or more exporters. In addition, diagnostics can also be configured.
+YAML file. In general, at least one enabled receiver and one exporter needs to be configured.


Suggested change

YAML file. In general, at least one enabled receiver and one exporter needs to be configured.

YAML file. In general, at least one enabled receiver and one enabled exporter needs to be configured.

Thank you! Updating!

tigrannajaryan · 2019-07-30T14:03:24Z

README.md

-one or more exporters. In addition, diagnostics can also be configured.
+YAML file. In general, at least one enabled receiver and one exporter needs to be configured.
+
+*Note* This documentation is still in progress. For any questions, please reach out in the


tigrannajaryan · 2019-07-30T14:06:47Z

README.md

+- metrics: collects and processes metrics data.
+- traces: collects and processes trace data.
+
+A pipeline consists of a set of receivers, processors, and exporters. Each receiver/processor/exporter must be specified


Formatting suggestion: either stick to soft line breaks (i.e. only do hard line breaks between paragraphs) or use hard line breaks at around 80 characters in MD files. With longer lines (like 120 chars in this case) the side-by-side diff in Github doesn't look as nice unless you have a very large monitor.

Sounds good - I'll update.

To make things easier to catch especially with formatting, can we add a md formatter to catch all of these things?

There is an ongoing discussion on this topic: open-telemetry/opentelemetry-specification#192

After the decision is made we can look into having an md formatter to enforce the rules.

Great! That will be helpful to save everyone time and even have it as a precommit check!

tigrannajaryan · 2019-07-30T14:14:21Z

README.md

+- traces: collects and processes trace data.
+
+A pipeline consists of a set of receivers, processors, and exporters. Each receiver/processor/exporter must be specified
+in the configuration to be included in a pipeline and each receiver/processor/exporter can be used in more than one pipeline.


There is a slight nuance with processors that is worth mentioning explicitly.

When the same processor is referenced in multiple pipelines each pipelines gets a separate instance of that processor (each instance having the same config but separate internal state). By contrast receivers and exporters referenced from multiple pipelines are the same instance of the receiver and exporter.

So, for example if I have a "queue" processor referenced from 2 pipelines, it really means 2 separate queues, one for each pipeline.

While if I reference "OpenCensus" receiver from 2 pipeline, it is really the same receiver and the data it receives is duplicated and fed into 2 pipelines.

That is a good distinction that should be added. I will add it in this pr.
As the description said, this document change is only a starting point and I'm sure there is much missing from it. Once we have a good skeleton in place (probably needs another pr or two to get it solid), we can fill in remaining gaps.

tigrannajaryan · 2019-07-30T14:16:18Z

README.md

+The following is an example pipeline configuration. For more information, refer to [processor README.md](processor/README.md)
+```yaml
+pipelines:
+  traces/first:


The traces/first key syntax is a) slightly unconventional b) has meaning that is not immediately obvious. It is worth explaining the "type/name" key convention somewhere close to this place.

I'll remove '/first' and keep the example simple. This documentation will need a few more prs to make it a good landing page.

tigrannajaryan · 2019-07-30T14:43:50Z

receiver/opencensusreceiver/testdata/config.yaml

  opencensus/customname:
+    # The endpoint for this receiver will be: "0.0.0.0:9090".


I think it will be useful to mention that this is the endpoint the receiver will listen on.

tigrannajaryan · 2019-07-30T14:44:44Z

receiver/opencensusreceiver/testdata/config.yaml

    endpoint: 0.0.0.0:9090
+    # This receiver doesn't allow for back pressure.


I am not sure "allow" is the right word here. It should be more like "doesn't signal backpressure to the source". @pjanotti what do you think?

I like your suggestion @tigrannajaryan

tigrannajaryan · 2019-07-30T14:45:49Z

receiver/opencensusreceiver/testdata/config.yaml

    endpoint: 0.0.0.0:9090
+    # This receiver doesn't allow for back pressure.
+    disable-backpressure: true
+  # The following entry configures all of the keep alive settings. These settings are use during server initialization.


The following entry configures all of the keep alive settings.

This sounds like it is purely for configuring keep alive settings. It is a receiver which also shows how to configure keep alive settings.

These settings are use during server initialization.

Unclear. What "server"?

Server is referenced in several parts of the config and its documentation. I will update the description to say the settings are used to configure the underlying server used to connect to the endpoint.

I do not think we should be using the word "server" when referring to the receiver endpoint. If we want to refer to the whole executable we should use the word "service", if it is about the specific receiver then let's just say "receiver".

Using 'service' instead of 'server' is find with me. I will let other chime in since I have no strong feelings about it. However, if the final decision is to use 'service' instead of 'server' which is referenced in the config, sample yaml and comments across ocreceiver, I will change it in a follow up pr as to avoid making this pr any larger.

Synced offline - This was a misunderstanding on my part and Tigran's proposal is valid. I will update the comment to use receiver/service and leave mention of server to the details of the keep alive parameters.

receiver/opencensusreceiver/testdata/config.yaml

tigrannajaryan · 2019-07-30T14:49:10Z

receiver/opencensusreceiver/testdata/config.yaml

+      server-parameters:
+        max-connection-idle: 10s
+  # The following entry demonstrates how to specify TLS credentials for the server.
+  # Note: These are not valid files and will fail during server initialization.


These are not valid files

Perhaps better to say "these files do not exist"

… can be mix and matched.

ccaraman · 2019-08-01T18:54:09Z

I will merge master so i can get the mispell tool added to my local build

tigrannajaryan

LGTM.
In a separate PR please have a look at whether it makes sense to move enabled receivers check to validation code.

receiver/opencensusreceiver/testdata/config.yaml

* Migrate updates to Prometheus receiver from opencensus-service * Remove unnecessary configuration for adjusting metrics * Update exporters README (#210) Remove stale content, put in place a simple skeleton, and put information for the Zipkin exporter. * Added Jaeger gRPC receiver (#197) * 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> * Fix README - remove broken links and commands (#211) Signed-off-by: Hui Kang <kangh@us.ibm.com> * Refactor opencensus exporter to make it easily extensible (#212) 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` * Use sha256 instead of md5 in node batcher processor (#202) MD5 is insecure cryptographic hash functions, and are therefore unsuitable for security applications. * Add goimports check and fix import order for all files (#209) * 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 (#200) * 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 (#214) * Add misspell check and fix all typos Updates #155. * Format * Include yaml files * Move tail sampling config to its own package and remove stale configs (#217) Follow up of #200. Second step of #146. * Add Observability Vision document (#188) * 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 (#207) * 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 * Update README to address reviewer comments. * Update module dependencies and import order for metricfamily * Improve OC Receiver Documentation (#203) * 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 and fix all errors (#218) * Add static check Fixes #155. * Fix most staticcheck errors * More fixes * Fix id_batcher * Rename exporterhelp parameter (#220) The paramater was named exportFormat but it actually used as the exporter name. * Fix build: lower casing error message (#224) * Add jaeger grpc exporter (#219) * 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 * Migrate updates to Prometheus receiver from opencensus-service * Remove unnecessary configuration for adjusting metrics * Update README to address reviewer comments. * Update module dependencies and import order for metricfamily * Fix goimports * Fix staticcheck issues

ccaraman added 3 commits July 29, 2019 12:50

First round of receiver and opencensus receiver documentation.

96211b4

Merge branch 'master' into receiver_oc_getting_started

4a0c43f

Undo go mod/sum changes

c6b11e0

ccaraman requested review from bogdandrutu, flands, pjanotti, songy23 and tigrannajaryan as code owners July 29, 2019 20:00

songy23 reviewed Jul 29, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

config/config.go Show resolved Hide resolved

ccaraman commented Jul 29, 2019

View reviewed changes

receiver/README.md Outdated Show resolved Hide resolved

Address initial set of comments.

54e334c

pjanotti reviewed Jul 29, 2019

View reviewed changes

Address next set of comments.

bad4b8b

pjanotti approved these changes Jul 30, 2019

View reviewed changes

tigrannajaryan requested changes Jul 30, 2019

View reviewed changes

songy23 assigned ccaraman Jul 30, 2019

Address next set of comments.

7b1d161

ccaraman changed the title ~~OC Receiver Documentation: First Pass~~ Improve OC Receiver Documentation Jul 30, 2019

ccaraman added 2 commits August 1, 2019 11:41

Fix use of server instead of receiver in comment and explain settings…

048068a

… can be mix and matched.

Merge branch 'master' into receiver_oc_getting_started

c8d8d89

Merged master and fixed mispell error caugh with new tools

dbfbc7f

tigrannajaryan approved these changes Aug 1, 2019

View reviewed changes

receiver/opencensusreceiver/testdata/config.yaml Show resolved Hide resolved

pjanotti merged commit 1660277 into open-telemetry:master Aug 1, 2019

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023

bump contrib modules to main (open-telemetry#203)

471e8c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve OC Receiver Documentation #203

Improve OC Receiver Documentation #203

ccaraman commented Jul 29, 2019 •

edited

Loading

codecov-io commented Jul 29, 2019 •

edited

Loading

pjanotti left a comment

tigrannajaryan commented Jul 30, 2019

tigrannajaryan left a comment

tigrannajaryan Jul 30, 2019

ccaraman Jul 30, 2019

tigrannajaryan Jul 30, 2019

tigrannajaryan Jul 30, 2019

ccaraman Jul 30, 2019

tigrannajaryan Jul 30, 2019

ccaraman Aug 1, 2019

tigrannajaryan Jul 30, 2019

ccaraman Jul 30, 2019

tigrannajaryan Jul 30, 2019

ccaraman Jul 30, 2019

tigrannajaryan Jul 30, 2019

tigrannajaryan Jul 30, 2019

pjanotti Jul 30, 2019

tigrannajaryan Jul 30, 2019

ccaraman Jul 30, 2019

tigrannajaryan Jul 30, 2019

ccaraman Jul 30, 2019

ccaraman Aug 1, 2019

tigrannajaryan Jul 30, 2019

ccaraman commented Aug 1, 2019

tigrannajaryan left a comment

	YAML file. In general, at least one enabled receiver and one exporter needs to be configured.
	YAML file. In general, at least one enabled receiver and one enabled exporter needs to be configured.

		opencensus/customname:
		# The endpoint for this receiver will be: "0.0.0.0:9090".

		endpoint: 0.0.0.0:9090
		# This receiver doesn't allow for back pressure.

Improve OC Receiver Documentation #203

Improve OC Receiver Documentation #203

Conversation

ccaraman commented Jul 29, 2019 • edited Loading

codecov-io commented Jul 29, 2019 • edited Loading

Codecov Report

pjanotti left a comment

Choose a reason for hiding this comment

tigrannajaryan commented Jul 30, 2019

tigrannajaryan 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

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

ccaraman commented Aug 1, 2019

tigrannajaryan left a comment

Choose a reason for hiding this comment

ccaraman commented Jul 29, 2019 •

edited

Loading

codecov-io commented Jul 29, 2019 •

edited

Loading