Add support for UI configuration #115
Conversation
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #115 +/- ##
==========================================
- Coverage 92.28% 92.12% -0.16%
==========================================
Files 25 27 +2
Lines 1063 1143 +80
==========================================
+ Hits 981 1053 +72
- Misses 82 86 +4
- Partials 0 4 +4
Continue to review full report at Codecov.
|
|
I'll wait a couple of days to review/merge this one, as I'm currently working on #66, which will have a lot of changes, potentially conflicting with the ones from this PR. |
|
No problem - thanks. |
|
Looks like you just did. |
|
@jpkrohling Although running the e2e tests, looks like the UI test has failed - so need to investigate, not sure whether something has changed with the UI config format :( |
|
@jpkrohling When running just the new However when I uncommented all of the other tests, still using the locally built image, it started failing on that new UI test again. It appears that if I can't see any reason why the tests would interfere with each other - could you try running the same locally to see if you see the same results? |
|
@jpkrohling I found a workaround - just combining the two UI (base path and config) tests - so if you can't spot anything obvious I will commit this change. |
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @objectiser)
deploy/examples/all-in-one-with-options.yaml, line 16 at r1 (raw file):
options: dependencies: menuEnabled: false
Could it be named enabled instead? I know it's not technically correct, as it's just hiding the menu option, but I think consistency is a bit better than technical correctness in this case.
pkg/apis/io/v1alpha1/freeform.go, line 7 at r1 (raw file):
) // FreeForm defines a common options parameter to the different structs
I now understand why we have an Options object and a FreeForm, but I wonder if we could have one generic construct that would work for both cases. If not, it would probably be good to have a few words as documentation explaining why we need both.
pkg/apis/io/v1alpha1/freeform.go, line 9 at r1 (raw file):
// FreeForm defines a common options parameter to the different structs type FreeForm struct { json []byte
As your marshaling of the map doesn't have a custom logic, you could probably just have a type FreeForm map[string]interface{}.
pkg/configmap/ui.go, line 6 at r1 (raw file):
"fmt" "github.com/sirupsen/logrus"
Note to self: we have usage like this in some places, and we are aliasing it to log in others. We should do go over the code and be consistent. It looks like the best practice is to alias to log.
pkg/configmap/ui.go, line 30 at r1 (raw file):
} json, err := u.jaeger.Spec.UI.Options.MarshalJSON()
MarshalJSON is part of the interface that the json library uses. The usual way to get a JSON from an object is via json.Marshal(u.jaeger.Spec.UI.Options).
https://blog.golang.org/json-and-go
pkg/controller/jaeger/all-in-one.go, line 68 at r1 (raw file):
} // add the config map
Shouldn't this be created before the Query deployment? Probably doesn't matter much, as it will probably be ready before the Query can consume it.
pkg/controller/jaeger/production.go, line 74 at r1 (raw file):
} // add the config map
Same here: I would move it to before the // add the deployments section
test/e2e/all_in_one_test.go, line 234 at r1 (raw file):
} if !strings.Contains(string(body), TrackingID) {
Nice trick to check the content 👍
|
By the way: the e2e tests are failing to me. Besides the |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jpkrohling and @objectiser)
deploy/examples/all-in-one-with-options.yaml, line 16 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Could it be named
enabledinstead? I know it's not technically correct, as it's just hiding the menu option, but I think consistency is a bit better than technical correctness in this case.
All of the content under the "options:" node is the Jaeger UI configuration format: https://www.jaegertracing.io/docs/1.8/deployment/#UIConfiguration
pkg/configmap/ui.go, line 30 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
MarshalJSONis part of the interface that thejsonlibrary uses. The usual way to get a JSON from an object is viajson.Marshal(u.jaeger.Spec.UI.Options).
Will do
pkg/controller/jaeger/all-in-one.go, line 68 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Shouldn't this be created before the Query deployment? Probably doesn't matter much, as it will probably be ready before the Query can consume it.
Yes I think it would be ready - but if it did turn out to be a problem in the future, what would be the approach to use, to ensure thinks are set up in a particular order? Would it need to be defined in the Dependencies list?
pkg/controller/jaeger/production.go, line 74 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Same here: I would move it to before the
// add the deploymentssection
Sure, will change the order - although I assume it will be deployed at the same time as all of the other objects.
There was a problem hiding this comment.
Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on @jpkrohling and @objectiser)
deploy/examples/all-in-one-with-options.yaml, line 16 at r1 (raw file):
Previously, objectiser (Gary Brown) wrote…
All of the content under the "options:" node is the Jaeger UI configuration format: https://www.jaegertracing.io/docs/1.8/deployment/#UIConfiguration
My bad, sorry!
pkg/controller/jaeger/all-in-one.go, line 68 at r1 (raw file):
Previously, objectiser (Gary Brown) wrote…
Yes I think it would be ready - but if it did turn out to be a problem in the future, what would be the approach to use, to ensure thinks are set up in a particular order? Would it need to be defined in the Dependencies list?
Yes, using the dependencies would be appropriate if we are not sure the resource will be ready by the time it's consumed. A Route or ConfigMap is usually ready right away, even if it's not an atomic operation.
There was a problem hiding this comment.
Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on @jpkrohling and @objectiser)
pkg/apis/io/v1alpha1/freeform.go, line 9 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
As your marshaling of the map doesn't have a custom logic, you could probably just have a
type FreeForm map[string]interface{}.
Unfortunately does not work, due to generated code not liking interface{}: pkg/apis/io/v1alpha1/zz_generated.deepcopy.go:37:22: val.DeepCopyinterface undefined (type interface {} is interface with no methods)
pkg/configmap/ui.go, line 30 at r1 (raw file):
Previously, objectiser (Gary Brown) wrote…
Will do
Remember why it had to be done this way now - the type (FreeForm in this case) can't have a field with type interface{} as it causes a problem with the generated code. Hence using the wrapper to convert to byte[] straightaway and then return it via the MarshallJSON method.
pkg/controller/jaeger/all-in-one.go, line 68 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Yes, using the dependencies would be appropriate if we are not sure the resource will be ready by the time it's consumed. A
RouteorConfigMapis usually ready right away, even if it's not an atomic operation.
Done.
pkg/controller/jaeger/production.go, line 74 at r1 (raw file):
Previously, objectiser (Gary Brown) wrote…
Sure, will change the order - although I assume it will be deployed at the same time as all of the other objects.
Done.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @objectiser)
pkg/apis/io/v1alpha1/freeform.go, line 9 at r1 (raw file):
Previously, objectiser (Gary Brown) wrote…
Unfortunately does not work, due to generated code not liking
interface{}: pkg/apis/io/v1alpha1/zz_generated.deepcopy.go:37:22: val.DeepCopyinterface undefined (type interface {} is interface with no methods)
True, I think I bumped into that one as well with Options.
pkg/configmap/ui.go, line 30 at r1 (raw file):
Previously, objectiser (Gary Brown) wrote…
Remember why it had to be done this way now - the type (FreeForm in this case) can't have a field with type
interface{}as it causes a problem with the generated code. Hence using the wrapper to convert tobyte[]straightaway and then return it via theMarshallJSONmethod.
Does it mean that json.Marshal(freeForm) won't work? If so, could you check the output of kubectl get jaeger my-jaeger -o yaml for a my-jaeger that specifies a UI config? That YAML output is generated by Kubernetes calling json.Marshal(obj) at the top-level Jaeger object, which ends up doing the same for the FreeForm.
test/e2e/all_in_one_test.go, line 113 at r1 (raw file):
} err = WaitForIngress(t, f.KubeClient, namespace, "all-in-one-with-base-path-query", retryInterval, timeout)
This might have been the problem before: IIRC, I did have problems with ingresses not getting updated between tests.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @objectiser)
pkg/configmap/ui.go, line 30 at r1 (raw file):
Previously, jpkrohling (Juraci Paixão Kröhling) wrote…
Does it mean that
json.Marshal(freeForm)won't work? If so, could you check the output ofkubectl get jaeger my-jaeger -o yamlfor amy-jaegerthat specifies a UI config? That YAML output is generated by Kubernetes callingjson.Marshal(obj)at the top-level Jaeger object, which ends up doing the same for theFreeForm.
Just tried doing the update you suggested, but has the following test failure:
--- FAIL: TestWithUIConfig (0.00s)
ui_test.go:41:
Error Trace: ui_test.go:41
Error: Not equal:
expected: "{\"tracking\":{\"gaID\":\"UA-000000-2\"}}"
actual : "{}"
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-{"tracking":{"gaID":"UA-000000-2"}}
+{}
Test: TestWithUIConfig
The output from the kubectl get jaeger my-jaeger -o yaml command, using the current approach is:
apiVersion: io.jaegertracing/v1alpha1
kind: Jaeger
metadata:
annotations:
kubectl.kubernetes.io/last-applied-configuration: |
{"apiVersion":"io.jaegertracing/v1alpha1","kind":"Jaeger","metadata":{"annotations":{},"name":"my-jaeger","namespace":"default"},"spec":{"allInOne":{"image":"jaegertracing/all-in-one:1.8","options":{"log-level":"debug","query":{"base-path":"/jaeger"}}},"storage":{"options":{"memory":{"max-traces":100000}}},"strategy":"allInOne","ui":{"options":{"dependencies":{"menuEnabled":false},"menu":[{"items":[{"label":"Documentation","url":"https://www.jaegertracing.io/docs/latest"}],"label":"About Jaeger"}],"tracking":{"gaID":"UA-000000-2"}}}}}
clusterName: ""
creationTimestamp: 2018-11-21T12:13:47Z
generation: 1
name: my-jaeger
namespace: default
resourceVersion: "610"
selfLink: /apis/io.jaegertracing/v1alpha1/namespaces/default/jaegers/my-jaeger
uid: e645eb1a-ed86-11e8-a975-70277121420b
spec:
agent:
image: jaegertracing/jaeger-agent:1.8
options: {}
resources: {}
strategy: ""
volumeMounts: null
volumes: null
allInOne:
image: jaegertracing/all-in-one:1.8
options:
log-level: debug
query.base-path: /jaeger
resources: {}
volumeMounts: null
volumes: null
collector:
image: ""
options: {}
resources: {}
size: 0
volumeMounts: null
volumes: null
ingress:
enabled: null
resources: {}
security: ""
volumeMounts: null
volumes: null
query:
image: ""
options: {}
resources: {}
size: 0
volumeMounts: null
volumes: null
resources: {}
storage:
cassandraCreateSchema:
datacenter: ""
enabled: null
image: ""
mode: ""
options:
memory.max-traces: "100000"
type: memory
strategy: allInOne
ui:
options:
dependencies:
menuEnabled: false
menu:
- items:
- label: Documentation
url: https://www.jaegertracing.io/docs/latest
label: About Jaeger
tracking:
gaID: UA-000000-2
volumeMounts: null
volumes: null
status: {}
So looks like it is fine.
Signed-off-by: Gary Brown <[email protected]>
Signed-off-by: Gary Brown <[email protected]>
…figmap in the controllers Signed-off-by: Gary Brown <[email protected]>
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @objectiser)
pkg/configmap/ui.go, line 30 at r1 (raw file):
Previously, objectiser (Gary Brown) wrote…
Just tried doing the update you suggested, but has the following test failure:
--- FAIL: TestWithUIConfig (0.00s) ui_test.go:41: Error Trace: ui_test.go:41 Error: Not equal: expected: "{\"tracking\":{\"gaID\":\"UA-000000-2\"}}" actual : "{}" Diff: --- Expected +++ Actual @@ -1 +1 @@ -{"tracking":{"gaID":"UA-000000-2"}} +{} Test: TestWithUIConfigThe output from the
kubectl get jaeger my-jaeger -o yamlcommand, using the current approach is:apiVersion: io.jaegertracing/v1alpha1 kind: Jaeger metadata: annotations: kubectl.kubernetes.io/last-applied-configuration: | {"apiVersion":"io.jaegertracing/v1alpha1","kind":"Jaeger","metadata":{"annotations":{},"name":"my-jaeger","namespace":"default"},"spec":{"allInOne":{"image":"jaegertracing/all-in-one:1.8","options":{"log-level":"debug","query":{"base-path":"/jaeger"}}},"storage":{"options":{"memory":{"max-traces":100000}}},"strategy":"allInOne","ui":{"options":{"dependencies":{"menuEnabled":false},"menu":[{"items":[{"label":"Documentation","url":"https://www.jaegertracing.io/docs/latest"}],"label":"About Jaeger"}],"tracking":{"gaID":"UA-000000-2"}}}}} clusterName: "" creationTimestamp: 2018-11-21T12:13:47Z generation: 1 name: my-jaeger namespace: default resourceVersion: "610" selfLink: /apis/io.jaegertracing/v1alpha1/namespaces/default/jaegers/my-jaeger uid: e645eb1a-ed86-11e8-a975-70277121420b spec: agent: image: jaegertracing/jaeger-agent:1.8 options: {} resources: {} strategy: "" volumeMounts: null volumes: null allInOne: image: jaegertracing/all-in-one:1.8 options: log-level: debug query.base-path: /jaeger resources: {} volumeMounts: null volumes: null collector: image: "" options: {} resources: {} size: 0 volumeMounts: null volumes: null ingress: enabled: null resources: {} security: "" volumeMounts: null volumes: null query: image: "" options: {} resources: {} size: 0 volumeMounts: null volumes: null resources: {} storage: cassandraCreateSchema: datacenter: "" enabled: null image: "" mode: "" options: memory.max-traces: "100000" type: memory strategy: allInOne ui: options: dependencies: menuEnabled: false menu: - items: - label: Documentation url: https://www.jaegertracing.io/docs/latest label: About Jaeger tracking: gaID: UA-000000-2 volumeMounts: null volumes: null status: {}So looks like it is fine.
There's clearly something I'm missing, but it does look like it's working.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @objectiser)
There was a problem hiding this comment.
Dismissed @objectiser from a discussion.
Reviewable status:complete! all files reviewed, all discussions resolved
No description provided.