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

[BUG] Server exporter config not match document and fail to achieve same flexability after refactor #4157

Closed
2 of 3 tasks
jiekun opened this issue Sep 6, 2023 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@jiekun
Copy link
Contributor

jiekun commented Sep 6, 2023

Search before asking

  • I had searched in the issues and found no similar feature requirement.

DeepFlow Component

Server

What you expected to happen

It seems the document is not updated after #4050: https://github.com/deepflowio/docs/blob/main/docs/zh/07-server-integration/02-export/02-opentelemetry-collector.md

Also, there are some small issues with this refactor:

  1. The new config layout removed the ability of exporting to multiple component with same protocol, for example 2 OTLP collector;
  2. ExportDatas and ExportDataTypes are now shared by all exporters. How could user exporter different flow_log to other component with the expectation of different data types? For example, in OTLP exporter we want ebpf-sys-span only and in Loki exporter we want cbpf-net-span + ebpf-sys-span.

How to reproduce

No response

DeepFlow version

No response

DeepFlow agent list

No response

Kubernetes CNI

No response

Operation-System/Kernel version

No response

Anything else

No response

Are you willing to submit a PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@jiekun jiekun added the bug Something isn't working label Sep 6, 2023
@jiekun jiekun changed the title [BUG] [BUG] Server exporter config not match document and fail to achieve same flexability before refactor Sep 6, 2023
@lzf575
Copy link
Contributor

lzf575 commented Sep 7, 2023

@jiekun
Copy link
Contributor Author

jiekun commented Sep 7, 2023

Why not raise those point of views in the original PR ( #3932 )? We could have the config layout discussed and solve it in time. And if it's not met the requirement and defination, we should fix it during code review, not rushing to merge it and raise another PR quickly.
为什么不在之前的 PR ( #3932 )中提出这些意见呢?我们可以讨论配置的格式并且及时调整它。如果配置不符合要求和定义,我们应该在 Code Review 的过程中去解决,而不是赶着合并然后短时间内重新开一个 PR 去改它。

We are already developing based on the previous config layout. It's quite common sending o11y data to both cloud provider infrastructure and internal platform at the same time, with different config to filter out different types of span.
我们已经在基于之前的配置格式开发了一些 exporter,将 o11y 数据同时发往云服务提供商的平台和内部平台是很常见的,以不同的配置来过滤不同的数据也一样。

Last but not least, it's not the case that we develop a new config layout (like the current one), which means we could have more optimization later. We are changing a config layout that ALREADY provide higher flexability to a more limited one. I mean, it's not a bad thing to provide and retain the flexability. I am not sure why this should not be discussed in issue (or maybe we are just discussing now).
多说一句,这并不是新设计一份配置格式的 case,如果是那样的话我们可以说“后续还会继续优化”。我们是在将已经拥有很好的扩展性的配置方案(当然其中有一些内容,例如 universal tag 是公共的需要被移至其他目录)降级成了更受限的格式。提供(正确来说是保留)好的扩展性应该不是一件坏事才对。我不确定这样的改动为何没有任何 Review Comment 或者说没有任何 Issue 讨论就能被 Approve(也可能是现在就在讨论中)。

@jiekun jiekun changed the title [BUG] Server exporter config not match document and fail to achieve same flexability before refactor [BUG] Server exporter config not match document and fail to achieve same flexability after refactor Sep 7, 2023
@lzf575
Copy link
Contributor

lzf575 commented Sep 7, 2023

你提的PR( #3932 )已经是完整的了,当时是无需再处理的。
后续考虑到exporters一些配置/模块是公共的,可以提取出来,做了些优化,对接口变化实际不大,主要是代码位置调整有些多。目前的扩展还不是最终的,后续还会继续优化,也欢迎直接提pr优化。
目前会根据你的意见将当前的公共配置,由各expoerter自行处理。

@jiekun
Copy link
Contributor Author

jiekun commented Sep 7, 2023

So some of the questions have conclusion (and maybe some advices) here.
1.The following (shared) params can be overwritten by exporter internal params. We can add a new struct and put it to both exporters config and exporter config.

	ExportDatas                 []string `yaml:"export-datas"`
	ExportDataBits              uint32   // generate from 'ExportDatas'
	ExportDataTypes             []string `yaml:"export-data-types"`
	ExportDataTypeBits          uint32   // generate from 'ExportDataTypes'
	ExportCustomK8sLabelsRegexp string   `yaml:"export-custom-k8s-labels-regexp"`
	ExportOnlyWithTraceID       bool     `yaml:"export-only-with-traceid"`
  1. Configuring multiple exporters under the same type (e.g. otlp exporter) is still not supported. Please consider it and feel free to discuss.

@jiekun
Copy link
Contributor Author

jiekun commented Sep 7, 2023

/todo doc update

@lzf575
Copy link
Contributor

lzf575 commented Sep 7, 2023

So some of the questions have conclusion (and maybe some advices) here. 1.The following (shared) params can be overwritten by exporter internal params. We can add a new struct and put it to both exporters config and exporter config.

	ExportDatas                 []string `yaml:"export-datas"`
	ExportDataBits              uint32   // generate from 'ExportDatas'
	ExportDataTypes             []string `yaml:"export-data-types"`
	ExportDataTypeBits          uint32   // generate from 'ExportDataTypes'
	ExportCustomK8sLabelsRegexp string   `yaml:"export-custom-k8s-labels-regexp"`
	ExportOnlyWithTraceID       bool     `yaml:"export-only-with-traceid"`
  1. Configuring multiple exporters under the same type (e.g. otlp exporter) is still not supported. Please consider it and feel free to discuss.

第一点你已经修改了
第二点目前需要定义配置为数组来实现(#4177)
如果多个同类型的exporter的配置是一致的,后续需要定义 addr为数组,支持同时向多个地址export, 目前每个地址都生成一个exporter,exporter数量多的话,会加剧内存和cpu的消耗。

@jiekun
Copy link
Contributor Author

jiekun commented Sep 7, 2023

第一点你已经修改了 第二点目前需要定义配置为数组来实现(#4177) 如果多个同类型的exporter的配置是一致的,后续需要定义 addr为数组,支持同时向多个地址export, 目前每个地址都生成一个exporter,exporter数量多的话,会加剧内存和cpu的消耗。

I still suggest making them configurable separately, similar to what we had before, or referring to the OpenTelemetry processor approach: link to OpenTelemetry Tailsampling Processor README ↗.
我仍想建议让他们完全可独立配置,而非在某一类配置中取巧,就像之前的版本,或者 OpenTelemetry Processor 一样。

The key point is that each exporter instance should be able to work independently, without being affected by the configuration of other exporters.
重点是,每个 exporter 实例应该具备独立运行的可能,不受任何其他的 exporter 影响(当然如果有一些类似 universal tag 的问题除外,至少对于 addr、filter 等应该是相互独立的)。

The current approach still doesn't achieve the goal of sending different types of data to different receivers under the same protocol or export type. For example, sending basic trace data to an external platform like Datadog or New Relic, while sending all data to an internal platform.
在 addr 上取巧的做法不能解决给不同同类 receviver 提供不同 trace data 的问题,例如,向 Data Dog / New Relic 发送基础的 Trace 数据,往内部平台发送所有的 Trace 数据。

Also, we must consider the cost of hardware resources. Users need to be aware that running an additional exporter will incur an additional 100% cost. I assume this is easily understandable. However, sharing the same configuration doesn't reduce the cost, while fully separating them can take advantage of various benefits.
当然,我们需要考虑资源成本,用户需要知道每运行一个额外的 exporter 会有翻倍的开销,这应该很容易理解。然而,对 addr 下手的方案对解决这样的问题并没有帮助,甚至应该说每个 exporter 拥有独立配置更易于做到精细控制。

If you consider it necessary, we can also provide something like "multiple exporter under same type, while having multiple addr for each exporter"...so that we can save a lot of time to handle situation like: 10 exporters with 10 addrs, while 5 of them need smaller trace dataset and others need full trace data. (But yes it's over designed.)
如果有需要,也可以两种方式结合,同类exporter可以有多个,各自有不同配置,而同配置的exporter可以发往多个 addr,去解决一些非常极端的问题(当然,这是过度设计,但是上一段所说的不是,因为发往不同 receiver 的根因正是因为他们需要不同数据,否则一套平台就可以完全支撑了。多 addr 的设计没有解决核心的问题,而只是个表面上支持群发的方法,这会导致在需要群发时,需要filter多平台的并集数据,每个平台都需要收到超出他们期望的数据,再各自进行过滤)。

I am going to provide config layout for two solution we discussed above, and see if others want to discuss and come up with a conclusion.

Layout 1

  exporters:
    enabled: true
    export-datas: [cbpf-net-span,ebpf-sys-span]
    otlp-exporters:
    - enabled: false
      addr: [127.0.0.1:4317, www.my_sw_receiver.com]
      queue-count: 4
      ...

Layout 2

  exporters:
    export-datas: [cbpf-net-span,ebpf-sys-span]
    - name: tencent-otlp-exporter
      type: otlp-exporter
      otlp-exporter-config:
        addr: 127.0.0.1:4317 
        queue-count: 4       
    - name: skywalking-platform-exporter
      type: otlp-exporter
      otlp-exporter-config:
        addr: www.my_sw_receiver.com
        queue-count: 8

@jiekun
Copy link
Contributor Author

jiekun commented Sep 7, 2023

I'm reviewing the code changes again. And found that seperate configuration for each exporter it's already supported. Sorry for the misunderstanding.

@jiekun
Copy link
Contributor Author

jiekun commented Sep 8, 2023

This is refactored and solved in #4177 . Docs updated as well. I'm closing the issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants