Skip to content

Conversation

@dmitryax
Copy link
Member

@dmitryax dmitryax commented Sep 14, 2020

Link to tracking Issue: #227

More TODO:

  • Other outputs
  • Docs
  • Tests
  • Fill values.schema.json

@dmitryax dmitryax added the WIP label Sep 14, 2020
@dmitryax dmitryax force-pushed the helm-chart branch 2 times, most recently from 14471af to 9a7ffc5 Compare September 14, 2020 07:36
Copy link
Member

@naseemkullah naseemkullah left a comment

Choose a reason for hiding this comment

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

Looks really good!

Left some comments about wether or not if both collector types are desired, how this should be achieved either by one helm release or two. Also that autoscaling enablement should also depend on standalone being deployed.

@tigrannajaryan
Copy link
Member

@dmitryax should we close this since there is a separate repo now?

@dmitryax
Copy link
Member Author

@tigrannajaryan I'd rather finalize the initial PR here to be approved, then close and resubmit it in the helm repo as is.

@codecov
Copy link

codecov bot commented Sep 25, 2020

Codecov Report

Merging #1026 into master will decrease coverage by 0.66%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1026      +/-   ##
==========================================
- Coverage   88.74%   88.08%   -0.67%     
==========================================
  Files         249      249              
  Lines       13295    11801    -1494     
==========================================
- Hits        11799    10395    -1404     
+ Misses       1141     1061      -80     
+ Partials      355      345      -10     
Flag Coverage Δ
#integration 73.63% <ø> (-1.25%) ⬇️
#unit 87.36% <ø> (-0.61%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
receiver/redisreceiver/interval/interval_runner.go 42.10% <0.00%> (-9.75%) ⬇️
...er/awsxrayreceiver/internal/translator/metadata.go 66.66% <0.00%> (-8.34%) ⬇️
...ver/kubeletstatsreceiver/kubelet/stats_provider.go 55.55% <0.00%> (-8.09%) ⬇️
receiver/redisreceiver/redis_metric.go 63.15% <0.00%> (-6.41%) ⬇️
receiver/receivercreator/receiver.go 67.64% <0.00%> (-6.17%) ⬇️
internal/common/testing/container/container.go 67.64% <0.00%> (-6.17%) ⬇️
receiver/receivercreator/observerhandler.go 60.37% <0.00%> (-5.76%) ⬇️
receiver/kubeletstatsreceiver/kubelet/cert.go 63.63% <0.00%> (-5.60%) ⬇️
receiver/receivercreator/runner.go 48.14% <0.00%> (-5.19%) ⬇️
receiver/kubeletstatsreceiver/kubelet/cpu.go 80.00% <0.00%> (-4.62%) ⬇️
... and 239 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 7b89268...ad826da. Read the comment docs.

@dmitryax dmitryax marked this pull request as ready for review September 25, 2020 08:07
@dmitryax dmitryax requested a review from a team September 25, 2020 08:07
Copy link
Member

@naseemkullah naseemkullah left a comment

Choose a reason for hiding this comment

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

lgtm, I look forward to trying this out once the helm repo is set up and hosting it!
I do have one non-blocking nit: WDYT about renaming the agent-collector component/agentCollector value to just agent?
The current name makes it sounds like it collects agents.

@dmitryax
Copy link
Member Author

dmitryax commented Sep 25, 2020

I do have one non-blocking nit: WDYT about renaming the agent-collector component/agentCollector value to just agent?
The current name makes it sounds like it collects agents.

I agree it sounds a bit confusing. I was trying to have some consistency, so used this pair standaloneCollector/agentCollector . Do you suggest standaloneCollector/agent instead?

@naseemkullah
Copy link
Member

naseemkullah commented Sep 25, 2020

I do have one non-blocking nit: WDYT about renaming the agent-collector component/agentCollector value to just agent?
The current name makes it sounds like it collects agents.

I agree it sounds a bit confusing. I was trying to have some consistency, so used this pair standaloneCollector/agentCollector . Do you suggest standaloneCollector/agent instead?

Ah I see, I would prefer any of these:

  • standaloneCollector/agent
  • standalone/agent
  • collector/agent

The latter is probably my favorite, despite the service itself having collector in the name but any of these are nicer imho

@dmitryax
Copy link
Member Author

dmitryax commented Sep 25, 2020

I would lean towards collector/agent as well.

But we might want to introduce another name for the standalone collector which doesn't conflict with otel collector itself. So far I've heard versions like gateway or concentrator ...

@open-telemetry/collector-approvers any thoughts/suggestions on the naming for collector deployed as daemonset against collector deployed as standalone deployment? ^

@dmitryax dmitryax removed the WIP label Sep 25, 2020
@owais
Copy link
Contributor

owais commented Sep 25, 2020

Some SDK libraries also use the term agent to refer to the auto-instrumentation agent for example, Java: https://github.com/open-telemetry/opentelemetry-java-instrumentation/

I guess .Net might do the same once they have auto-instrumentation. So probably better to not call the collector an agent? I don't think it's a huge deal but would be nice if we could use a specific name that can't be confused with other things in the opentelemetry ecosystem.

@keitwb
Copy link
Contributor

keitwb commented Sep 25, 2020

I like what @dmitryax did with standaloneCollector/agentCollector, in that it has the term collector in both. However, standalone isn't very descriptive since all of the collectors are technically "stand alone" (not paired with anything else to do it's job).

One thing that has always been confusing to me is the totally different terms used for the same application (agent vs collector) to describe different configurations. Probably calling both of them some variation of collector is the clearest. Maybe centralCollector and hostCollector.

@dmitryax
Copy link
Member Author

dmitryax commented Sep 25, 2020

BTW choosing the naming we also need to consider how to differentiate other resources names like configmaps etc for each of the components. I added -agent suffix for agentCollector components, and resources used by standaloneCollector don't have a suffix.

dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 26, 2020
dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 26, 2020
@dmitryax
Copy link
Member Author

I'm closing this PR in favor of open-telemetry/opentelemetry-helm-charts#2 .
I kept naming as is for now, and moved our discussion to the issue open-telemetry/opentelemetry-helm-charts#3

@dmitryax dmitryax closed this Sep 26, 2020
dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 27, 2020
dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 27, 2020
dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 27, 2020
dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 27, 2020
dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 27, 2020
dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 27, 2020
dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 27, 2020
dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 28, 2020
dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 28, 2020
dmitryax added a commit to dmitryax/opentelemetry-helm-charts that referenced this pull request Sep 28, 2020
dmitryax added a commit to open-telemetry/opentelemetry-helm-charts that referenced this pull request Sep 29, 2020
@dmitryax dmitryax deleted the helm-chart branch September 29, 2020 22:50
ljmsc referenced this pull request in ljmsc/opentelemetry-collector-contrib Feb 21, 2022
* Break up the oterror package

* Update use of ErrorHandler in project

* Update handler naming and comments
codeboten pushed a commit that referenced this pull request Nov 23, 2022
jelly-afk pushed a commit to jelly-afk/opentelemetry-collector-contrib that referenced this pull request Sep 8, 2025
…lemetry#1026)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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.

5 participants