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

Head sampling crd #1445

Merged
merged 12 commits into from
Aug 18, 2024
Merged

Head sampling crd #1445

merged 12 commits into from
Aug 18, 2024

Conversation

tamirdavid1
Copy link
Collaborator

I've added @RonFed CRD additions for Head-sampling + change in the relevant places in the code.

Copy link
Collaborator

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

LGTM.
added few nit suggestions about updating someplace to the new names, and consider removing the contains operand for now, as it might get us into problems with eBPF implementation which we can postpone

api/odigos/v1alpha1/instrumentatioconfig_types.go Outdated Show resolved Hide resolved
api/odigos/v1alpha1/instrumentatioconfig_types.go Outdated Show resolved Hide resolved
api/odigos/v1alpha1/instrumentatioconfig_types.go Outdated Show resolved Hide resolved
api/odigos/v1alpha1/instrumentatioconfig_types.go Outdated Show resolved Hide resolved
api/odigos/v1alpha1/instrumentatioconfig_types.go Outdated Show resolved Hide resolved
api/odigos/v1alpha1/instrumentatioconfig_types.go Outdated Show resolved Hide resolved
@blumamir blumamir mentioned this pull request Aug 16, 2024
@edeNFed
Copy link
Contributor

edeNFed commented Aug 18, 2024

Great job @tamirdavid1
A few comments on naming (nothing really important, feel free to skip all of them):

  • .spec.config and .spec.sdkConfigs is confusing. if I understand correctly, one is for sampling and the other is for dynamic instrumentation, so the names should reflect that (sampling/dynamicAttributes/etc)
  • All the nested objects are fields in InstrumentationConfig object, I think we can drop the word instrumentation in multiple places in the json fields as it clear that everything is related to instrumentation.
  • operandsdoes not make it clear that there is an AND between all of them and that all should return true in order to pass. Maybe we can give it a better name to explain this behavior

Another thing to consider:
I see both .spec.config and .spec.sdkConfigs eventually point to a list of target libraries,
Would it make more sense to unify them both and make the libraries the root object?
Maybe something like:

spec:
    runtimeDetailsInvalidated: false
    configs:
        - name: "net/http"
          traceConfig:
              enbaled: true
          headSampling:
              fallbackFraction: 0.4
              rules:
                  - fraction: 1
                    operands:
                        - key: "http.method"
                          operation: "equal"
                          value: "POST"
          dynamicAttributes:
              - spanKind: "server"
                optionValueBoolean: true
                optionKey: "http.request.body"
    global:
        headSampling:
            fallbackFraction: 0.9

It is a big change and I am not sure how much value it adds, but maybe now is a good time to do it if you find it valuable. (cc @blumamir @RonFed )

@tamirdavid1 tamirdavid1 merged commit db0f59f into odigos-io:main Aug 18, 2024
20 checks passed
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.

4 participants