-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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] Add support for Elasticsearch #5152
Changes from 19 commits
0afdfd5
c60bf5a
67707ed
a47bb35
5ea1df1
70edeeb
430a0ce
d8d4939
da38c6a
001e4a0
f93b1c6
9aa2c26
c5c20ce
bcdf8a0
236067c
72c5c42
79130b4
e373c3d
e646a3f
e61cf78
6824ec4
999d336
66f63b5
a86760c
7ad3b0d
8dc35cb
0744562
275786c
3763aa9
495daaf
0f7d2f1
a6cec26
915e83f
278d49a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
FROM alpine:latest | ||
|
||
WORKDIR /app | ||
|
||
COPY jaeger-linux-amd64 /app/ | ||
|
||
RUN chmod +x /app/jaeger-linux-amd64 | ||
|
||
EXPOSE 5775/udp 6831/udp 6832/udp 5778 16686 14268 14250 9200 | ||
|
||
ENTRYPOINT ["/app/jaeger-linux-amd64"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
version: '3.7' | ||
|
||
services: | ||
elasticsearch: | ||
image: elasticsearch:8.12.0 | ||
container_name: elasticsearch | ||
environment: | ||
- discovery.type=single-node | ||
- xpack.security.enabled=false | ||
ports: | ||
- "9200:9200" | ||
healthcheck: | ||
test: ["CMD-SHELL", "curl -s http://localhost:9200/_cluster/health | grep -vq '\"status\":\"red\"'"] | ||
interval: 10s | ||
timeout: 5s | ||
retries: 5 | ||
networks: | ||
- esnet | ||
|
||
jaeger: | ||
build: | ||
context: ./ | ||
dockerfile: Dockerfile.debug | ||
container_name: jaeger | ||
ports: | ||
- "16686:16686" | ||
- "6831:6831/udp" | ||
- "6832:6832/udp" | ||
- "5778:5778" | ||
- "14268:14268" | ||
- "14250:14250" | ||
depends_on: | ||
elasticsearch: | ||
condition: service_healthy | ||
networks: | ||
- esnet | ||
volumes: | ||
- ./es_config.yaml:/app/es_config.yaml | ||
- ./config-ui.json:/app/cmd/jaeger/config-ui.json | ||
command: ["--config", "/app/es_config.yaml"] | ||
restart: always | ||
|
||
networks: | ||
esnet: | ||
driver: bridge |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
service: | ||
akagami-harsh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
extensions: [jaeger_storage, jaeger_query] | ||
pipelines: | ||
traces: | ||
receivers: [otlp] | ||
processors: [batch] | ||
exporters: [jaeger_storage_exporter] | ||
|
||
extensions: | ||
jaeger_query: | ||
trace_storage: es_main | ||
trace_storage_archive: es_archive | ||
ui_config: ./cmd/jaeger/config-ui.json | ||
|
||
jaeger_storage: | ||
elasticsearch: | ||
es_main: | ||
server_urls: http://elasticsearch:9200 | ||
log_level: "error" | ||
num_shards: 5 | ||
es_archive: | ||
yurishkuro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
server_urls: http://elasticsearch:9200 | ||
log_level: "error" | ||
index_prefix: "jaeger-archive" | ||
num_shards: 5 | ||
receivers: | ||
otlp: | ||
protocols: | ||
grpc: | ||
http: | ||
|
||
processors: | ||
batch: | ||
|
||
exporters: | ||
jaeger_storage_exporter: | ||
trace_storage: es_main |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,7 @@ import ( | |
|
||
// Configuration describes the configuration properties needed to connect to an ElasticSearch cluster | ||
type Configuration struct { | ||
Servers []string `mapstructure:"server_urls"` | ||
Servers []string `mapstructure:"server_urls" validate:"required,min=1,dive,url"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if there's a test we could write for ensuring that these annotations are correct (btw, what is "dive"?) We could do it brute-force and just write individual tests for individual fields, but it seems redundant as that would be already done in the validator package (hopefully). But at the same time, having no tests to even ensure that the syntax of the annotations is correct is troublesome. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment for L58 and below - can you try removing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
removed it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
dive instructs the validation library to examine each element within the slice individually There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
we can write unit tests that check whether the validation behaves as expected. Instead of testing individual fields, we can create a set of test cases that cover various scenarios, including both valid and invalid configurations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but that wouldn't be what I am after, such test would simply verify that the Validator is working as expected, on select fields. But if I declare a field with annotations that don't make sense, I want to catch that in unit tests. E.g. misspell There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can use the "github.com/go-playground/validator" library in our unit tests to detect misspelled annotations and fields with invalid annotation combinations. like this package main
import (
"github.com/go-playground/validator"
)
type Webhook struct {
CallbackURL string `validate:"url,badAnnotation"` // badAnnotation is not a valid tag
}
func main() {
webhook := Webhook{
CallbackURL: "https://www.example.com/",
}
validate := validator.New()
validate.Struct(webhook)
}
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about "github.com/asaskevich/govalidator", will it do the same? We do not use "github.com/go-playground/validator" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Nope, it doesn't do the same. I actually tried using the existing validator at first. |
||
RemoteReadClusters []string `mapstructure:"remote_read_clusters"` | ||
Username string `mapstructure:"username"` | ||
Password string `mapstructure:"password" json:"-"` | ||
|
@@ -54,13 +54,13 @@ type Configuration struct { | |
AllowTokenFromContext bool `mapstructure:"-"` | ||
Sniffer bool `mapstructure:"sniffer"` // https://github.com/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 `yaml:"max_span_age" mapstructure:"-"` // configures the maximum lookback on span reads | ||
akagami-harsh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
NumShards int64 `yaml:"shards" mapstructure:"num_shards"` | ||
NumReplicas int64 `yaml:"replicas" mapstructure:"num_replicas"` | ||
PrioritySpanTemplate int64 `yaml:"priority_span_template" mapstructure:"priority_span_template"` | ||
PriorityServiceTemplate int64 `yaml:"priority_service_template" mapstructure:"priority_service_template"` | ||
PriorityDependenciesTemplate int64 `yaml:"priority_dependencies_template" mapstructure:"priority_dependencies_template"` | ||
MaxDocCount int `mapstructure:"-"` | ||
MaxSpanAge time.Duration `mapstructure:"-"` | ||
NumShards int64 `mapstructure:"num_shards"` | ||
NumReplicas int64 `mapstructure:"num_replicas"` | ||
PrioritySpanTemplate int64 `mapstructure:"priority_span_template"` | ||
PriorityServiceTemplate int64 `mapstructure:"priority_service_template"` | ||
PriorityDependenciesTemplate int64 `mapstructure:"priority_dependencies_template"` | ||
Timeout time.Duration `validate:"min=500" mapstructure:"-"` | ||
BulkSize int `mapstructure:"-"` | ||
BulkWorkers int `mapstructure:"-"` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,6 +83,24 @@ func NewFactory() *Factory { | |
} | ||
} | ||
|
||
func NewFactoryWithConfig( | ||
cfg config.Configuration, | ||
metricsFactory metrics.Factory, | ||
logger *zap.Logger, | ||
) (*Factory, error) { | ||
f := NewFactory() | ||
cfg.MaxDocCount = defaultMaxDocCount | ||
f.InitFromOptions(Options{ | ||
Primary: namespaceConfig{Configuration: cfg}, | ||
others: make(map[string]*namespaceConfig), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder, if only Primary and not Archive is ever initialized, would it actually provide an archive storage to the UI? Did you try it out? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, you are right it dose not work with ui. it throw's this error docker container used to test it
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better to look in the server logs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you put together a docker-compose that illustrates this issue? This "all shards failed" error has always annoyed me, I wonder if there are more details in the error returned from ES that we're not logging that could be more descriptive. Like, it's ridiculous for a database to respond with "your query failed" instead of telling why (the fact that it failed on "all shards" is not an explanation of why). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
sure, i'll make one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't check this one previously because there were no errors in the UI. However, I've just ran it with elasticsearch and found this issue.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when i checked with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least for memstore you need to add archive to the config explicitly: diff --git a/cmd/jaeger/internal/all-in-one.yaml b/cmd/jaeger/internal/all-in-one.yaml
index d17c52fc..a6189c8e 100644
--- a/cmd/jaeger/internal/all-in-one.yaml
+++ b/cmd/jaeger/internal/all-in-one.yaml
@@ -9,11 +9,14 @@ service:
extensions:
jaeger_query:
trace_storage: memstore
+ trace_storage_archive: memstore2
jaeger_storage:
memory:
memstore:
max_traces: 100000
+ memstore2:
+ max_traces: 100000 But maybe this is ok - the idea in v2 was to decouple primary and archive storage altogether, so that one could use different types of backends for those. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So in theory the config you have should've worked, because you did define archive storage, but something isn't working. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added a small fix for above problem, which sets |
||
}) | ||
err := f.Initialize(metricsFactory, logger) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return f, nil | ||
} | ||
|
||
// AddFlags implements plugin.Configurable | ||
func (f *Factory) AddFlags(flagSet *flag.FlagSet) { | ||
f.Options.AddFlags(flagSet) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why you need this one - how is it different from the main Dockerfile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was having issues with the main Dockerfile so i created a simpler one as a solution. now we don't need the Dockerfile and Docker-compose file anymore, so I'm removing them.