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

[jaeger-v2] Refactor ElasticSearch/OpenSearch Storage Configurations To Align With OTEL #6059

Closed
6 tasks done
mahadzaryab1 opened this issue Oct 6, 2024 · 9 comments · Fixed by #6090
Closed
6 tasks done

Comments

@mahadzaryab1
Copy link
Contributor

mahadzaryab1 commented Oct 6, 2024

Requirement

In this issue, we want to align the configuration for elastic search and open search storage with OTEL (part of #5229)

Problem

  • All config fields are tagged with mapstructure such that they can be addressable from YAML via "good" names
  • Configs implement Validate()
  • Configs via YAML support the same defaults as in jaeger-v1
  • Configs reuse standard elements of OTEL configs whenever possible, e.g. TLS, clientcfg, etc.
  • Configuration looks "logical" and properly grouped, not flattened with long weird names
  • We create a migration guide document with a table that lists v1 CLI flags and the corresponding v2 config fields
@mahadzaryab1
Copy link
Contributor Author

@yurishkuro is a migration guide needed here?

@yurishkuro
Copy link
Member

yes, certainly

yurishkuro added a commit that referenced this issue Oct 12, 2024
…6060)

## Which problem is this PR solving?
- Part of #6059

## Description of the changes
- Continuation of #5790
- Refactors the configurations for ElasticSearch and OpenSearch to
simplify the configuration by having more logical groupings

## How was this change tested?
- Unit Tests and E2E Integration Tests

## Checklist
- [x] I have read
https:/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Jared Tan <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Jared Tan <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
@mahadzaryab1
Copy link
Contributor Author

@yurishkuro this is the current configuration we have

type Configuration struct {
	Servers                  []string               `mapstructure:"server_urls" valid:"required,url"`
	RemoteReadClusters       []string               `mapstructure:"remote_read_clusters"`
	Username                 string                 `mapstructure:"username"`
	Password                 string                 `mapstructure:"password" json:"-"`
	TokenFilePath            string                 `mapstructure:"token_file"`
	PasswordFilePath         string                 `mapstructure:"password_file"`
	AllowTokenFromContext    bool                   `mapstructure:"-"`
	Sniffer                  bool                   `mapstructure:"sniffer"` // https:/olivere/elastic/wiki/Sniffing
	SnifferTLSEnabled        bool                   `mapstructure:"sniffer_tls_enabled"`
	MaxDocCount              int                    `mapstructure:"-"` // Defines maximum number of results to fetch from storage per query
	MaxSpanAge               time.Duration          `mapstructure:"-"` // configures the maximum lookback on span reads
	Timeout                  time.Duration          `mapstructure:"-"`
	BulkSize                 int                    `mapstructure:"-"`
	BulkWorkers              int                    `mapstructure:"-"`
	BulkActions              int                    `mapstructure:"-"`
	BulkFlushInterval        time.Duration          `mapstructure:"-"`
	Indices                  Indices                `mapstructure:"indices"`
	ServiceCacheTTL          time.Duration          `mapstructure:"service_cache_ttl"`
	AdaptiveSamplingLookback time.Duration          `mapstructure:"-"`
	Tags                     TagsAsFields           `mapstructure:"tags_as_fields"`
	Enabled                  bool                   `mapstructure:"-"`
	TLS                      configtls.ClientConfig `mapstructure:"tls"`
	UseReadWriteAliases      bool                   `mapstructure:"use_aliases"`
	CreateIndexTemplates     bool                   `mapstructure:"create_mappings"`
	UseILM                   bool                   `mapstructure:"use_ilm"`
	Version                  uint                   `mapstructure:"version"`
	LogLevel                 string                 `mapstructure:"log_level"`
	SendGetBodyAs            string                 `mapstructure:"send_get_body_as"`
}

Here are some groupings I see.

Sniffer

  • Enabled
  • TLS Enabled

Bulk Processing

  • Size
  • Num Workers
  • Num Actions
  • Flush Interval

Auth

Can be grouped under basic if we want to support more authentication types in the future

  • Username
  • Password
  • PasswordFilePath

Do you agree with the groupings above? Do you see any other groupings?

@yurishkuro
Copy link
Member

I wonder if we can get rid of these if we switch to data streams

UseReadWriteAliases
CreateIndexTemplates
UseILM

@yurishkuro
Copy link
Member

SendGetBodyAs - what are possible values? Should it be an enum?

@mahadzaryab1
Copy link
Contributor Author

SendGetBodyAs - what are possible values? Should it be an enum?

it looks to be all the http method types

// SetSendGetBodyAs specifies the HTTP method to use when sending a GET request
// with a body. It is GET by default.
func SetSendGetBodyAs(httpMethod string) ClientOptionFunc {
	return func(c *Client) error {
		c.sendGetBodyAs = httpMethod
		return nil
	}
}

@mahadzaryab1
Copy link
Contributor Author

I wonder if we can get rid of these if we switch to data streams

UseReadWriteAliases CreateIndexTemplates UseILM

what are data streams in this context?

Saumya40-codes pushed a commit to Saumya40-codes/jaeger that referenced this issue Oct 14, 2024
…aegertracing#6060)

## Which problem is this PR solving?
- Part of jaegertracing#6059

## Description of the changes
- Continuation of jaegertracing#5790
- Refactors the configurations for ElasticSearch and OpenSearch to
simplify the configuration by having more logical groupings

## How was this change tested?
- Unit Tests and E2E Integration Tests

## Checklist
- [x] I have read
https:/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Jared Tan <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Jared Tan <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: Saumya Shah <[email protected]>
Saumya40-codes pushed a commit to Saumya40-codes/jaeger that referenced this issue Oct 14, 2024
…aegertracing#6060)

## Which problem is this PR solving?
- Part of jaegertracing#6059

## Description of the changes
- Continuation of jaegertracing#5790
- Refactors the configurations for ElasticSearch and OpenSearch to
simplify the configuration by having more logical groupings

## How was this change tested?
- Unit Tests and E2E Integration Tests

## Checklist
- [x] I have read
https:/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Jared Tan <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Co-authored-by: Jared Tan <[email protected]>
Co-authored-by: Yuri Shkuro <[email protected]>
Signed-off-by: Saumya Shah <[email protected]>
@tronda
Copy link

tronda commented Oct 14, 2024

@mahadzaryab1 Amazon described the data stream feature in this video:
https://www.youtube.com/watch?v=Mm3GFTt8wMA

I think this comes from ElasticSearch originally so the concept is probably the same.

yurishkuro pushed a commit that referenced this issue Oct 14, 2024
…uration (#6079)

## Which problem is this PR solving?
- Towards #6059

## Description of the changes
- Migrated the ElasticSearch/OpenSearch configurations to use OTEL's TLS
configurations
- In a follow up PR, I'll re-evaluate the groupings of the
configurations and add the missing mapstructure tags

## How was this change tested?
- CI

## Checklist
- [x] I have read
https:/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

Signed-off-by: Mahad Zaryab <[email protected]>
@mahadzaryab1 mahadzaryab1 changed the title [jaeger-v2] Refactor ElasticSearch/OpenSearch Storage Configurations [jaeger-v2] Refactor ElasticSearch/OpenSearch Storage Configurations To Align With OTEL Oct 15, 2024
@mahadzaryab1
Copy link
Contributor Author

@yurishkuro final PR is ready for review at #6090

yurishkuro pushed a commit that referenced this issue Oct 19, 2024
## Which problem is this PR solving?
- Part of #6059

## Description of the changes
- Added some remaining documentation to the ElasticSearch configuration
that was left over from #6090

## How was this change tested?
- CI

## Checklist
- [x] I have read
https:/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

Signed-off-by: Mahad Zaryab <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
3 participants