OpenTelemetryCollector config files are different from reference #810

freschri · 2023-08-07T06:28:09Z

Please review

…blob/master/sample-configs/operator/collector-config-amp.yaml

shapirov103 · 2023-08-08T15:21:44Z

@elamaran11 please take a look, that was the manifest maintenance that we are on the hook for. If you have ideas on how we can better track this type of changes please let me know.

shapirov103

LGTM

shapirov103 · 2023-08-08T15:22:25Z

/do-e2e-tests

aws-ia-ci

end to end tests failed. A maintainer can provide more details.

shapirov103 · 2023-08-08T15:30:50Z

@freschri please merge main to address unit test failures:



Version was not specified by cluster builder or in cluster provider props, must be specified in one of these
--
250 |  
251 | 244 \|         const outputClusterName = true;
252 | 245 \|         if(!kubernetesVersion && !this.props.version) {
253 | > 246 \|             throw new Error("Version was not specified by cluster builder or in cluster provider props, must be specified in one of these");
254 | \|                   ^
255 | 247 \|         }
256 | 248 \|         const version: eks.KubernetesVersion = kubernetesVersion \|\| this.props.version \|\| eks.KubernetesVersion.V1_27;
257 | 249 \|

shapirov103

Unit test failures

elamaran11

LGTM

elamaran11 · 2023-08-08T15:43:22Z

@shapirov103 One thing we can definately do is load an externa YAML like how its done here and move these ytpl files to https://github.com/aws-observability/aws-observability-accelerator. We should partner with terraform accelerator to see if we can align both together. Im expecting this to take time and i also dont anticipate change to this file sooner. So lets have it like this for now. We can find a way to remove these yamls and load externally later.

…different-from-reference

freschri · 2023-08-08T17:10:19Z

@freschri please merge main to address unit test failures:



Version was not specified by cluster builder or in cluster provider props, must be specified in one of these
--
250 |  
251 | 244 \|         const outputClusterName = true;
252 | 245 \|         if(!kubernetesVersion && !this.props.version) {
253 | > 246 \|             throw new Error("Version was not specified by cluster builder or in cluster provider props, must be specified in one of these");
254 | \|                   ^
255 | 247 \|         }
256 | 248 \|         const version: eks.KubernetesVersion = kubernetesVersion \|\| this.props.version \|\| eks.KubernetesVersion.V1_27;
257 | 249 \|

done

elamaran11

@freschri Since you already have a PR for Collector config, Could you please add the config listed here for the API Troubleshooting board to work as you add the rules in CDK accelerator.

https://github.com/aws-observability/terraform-aws-observability-accelerator/blob/cc3fd5a8dda6edb69b1e444034c3b5687d654f87/modules/eks-monitoring/otel-config/templates/opentelemetrycollector.yaml#L72-L98

Also please add a flag to enable or disable API Server monitoring in Addon. We dont want this to be defaulted. Also please check this for adding documentation for API server mon.

elamaran11

Another comment. We need to add a feature to addon to allow external yaml like this This YAML should override the YAML part of the repo. Also this helps you to add the YAML to AWS Accelerator repo in a new folder like template and use that in the pattern.

freschri · 2023-08-11T07:08:18Z

Another comment. We need to add a feature to addon to allow external yaml like this This YAML should override the YAML part of the repo. Also this helps you to add the YAML to AWS Accelerator repo in a new folder like template and use that in the pattern.

I guess I did it in another PR see here: https://github.com/aws-quickstart/cdk-eks-blueprints/blob/64075bc726594e396d88f4d5b58a27a2dceee4a8/lib/addons/amp/index.ts#L33C6-L33C18
if that's what you mean

elamaran11 · 2023-08-11T12:02:12Z

Another comment. We need to add a feature to addon to allow external yaml like this This YAML should override the YAML part of the repo. Also this helps you to add the YAML to AWS Accelerator repo in a new folder like template and use that in the pattern.

I guess I did it in another PR see here: https://github.com/aws-quickstart/cdk-eks-blueprints/blob/64075bc726594e396d88f4d5b58a27a2dceee4a8/lib/addons/amp/index.ts#L33C6-L33C18 if that's what you mean

@freschri I know but i think you should be using loadExternalYaml function to read an external path. The manifestPath should be an internal code base path now inorder for this to work in the current code. If you use loadExternalYaml function, it will allow you to load from external path too and you can refactor later to use the external path in common gitops repo.

freschri · 2023-08-11T12:45:00Z

Another comment. We need to add a feature to addon to allow external yaml like this This YAML should override the YAML part of the repo. Also this helps you to add the YAML to AWS Accelerator repo in a new folder like template and use that in the pattern.

I guess I did it in another PR see here: https://github.com/aws-quickstart/cdk-eks-blueprints/blob/64075bc726594e396d88f4d5b58a27a2dceee4a8/lib/addons/amp/index.ts#L33C6-L33C18 if that's what you mean

@freschri I know but i think you should be using loadExternalYaml function to read an external path. The manifestPath should be an internal code base path now inorder for this to work in the current code. If you use loadExternalYaml function, it will allow you to load from external path too and you can refactor later to use the external path in common gitops repo.

ok can you please create an enhancement issue for that? not really related to the current changes and anyone can pick it up

elamaran11 · 2023-08-11T13:07:41Z

@freschri We already have an Issue for this. Since you are working on AmpAddon with this PR and you might need this feature for your proposal to CDK accelerator, i felt it is very relevant to add this low hanging fruit to this PR. I will leave it up to you to add to this work or a PR later. Thanks!

freschri · 2023-08-11T16:06:29Z

@freschri Since you already have a PR for Collector config, Could you please add the config listed here for the API Troubleshooting board to work as you add the rules in CDK accelerator.

https://github.com/aws-observability/terraform-aws-observability-accelerator/blob/cc3fd5a8dda6edb69b1e444034c3b5687d654f87/modules/eks-monitoring/otel-config/templates/opentelemetrycollector.yaml#L72-L98

Also please add a flag to enable or disable API Server monitoring in Addon. We dont want this to be defaulted. Also please check this for adding documentation for API server mon.

done

freschri · 2023-08-11T16:08:14Z

@freschri We already have an Issue for this. Since you are working on AmpAddon with this PR and you might need this feature for your proposal to CDK accelerator, i felt it is very relevant to add this low hanging fruit to this PR. I will leave it up to you to add to this work or a PR later. Thanks!

ok thanks then I will leave the work to be done in the context of Issue

elamaran11

LGTM. Minor feedback. Have a question on doc needs for string utils.

elamaran11 · 2023-08-11T18:50:22Z

lib/addons/amp/index.ts

@@ -90,7 +95,8 @@ export const enum DeploymentMode {
 const defaultProps = {
    deploymentMode: DeploymentMode.DEPLOYMENT,
    name: 'adot-collector-amp',
-    namespace: 'default'
+    namespace: 'default',
+    enableAPIserverJob: false


camelCase. enableAPIServerJob

elamaran11 · 2023-08-11T18:51:17Z

lib/utils/string-utils.ts

+ * @param string
+ * @returns
+ */
+export function changeTextBetweenTokens(str: string, openToken: string, closeToken: string, keep: boolean) {


i like this implementation. @shapirov103 Do we have to create a doc page for string utils?

We have not done it before, the utils part is not the main value of the blueprints. I would just put more emphasis on the JSDoc and rely on the API doc tool for those for now.

elamaran11 · 2023-08-15T14:24:59Z

@freschri Can you fix the issues, We are planning to release this week.

freschri · 2023-08-15T16:05:43Z

@freschri Can you fix the issues, We are planning to release this week.

what issues? all looks addressed above...

elamaran11 · 2023-08-15T16:53:58Z

@shapirov103 Could you review this PR?

elamaran11 · 2023-08-15T16:54:20Z

@freschri Can you fix the issues, We are planning to release this week.

what issues? all looks addressed above...

I see the issues are unresolved, Lets ask Mikhail to review and close this.

shapirov103 · 2023-08-16T13:42:54Z

/do-e2e-tests

aws-ia-ci

end to end tests passed

shapirov103

LGTM

shapirov103 · 2023-08-16T14:45:26Z

@elamaran11 your review status is "changes requested" - this prevents the merge atm.

elamaran11

LGTM

elamaran11 · 2023-08-16T14:56:03Z

@elamaran11 your review status is "changes requested" - this prevents the merge atm.

Merged.

updated from https://github.com/aws-observability/aws-otel-community/…

81eab0f

…blob/master/sample-configs/operator/collector-config-amp.yaml

freschri requested a review from aws-ia-ci as a code owner August 7, 2023 06:28

freschri linked an issue Aug 7, 2023 that may be closed by this pull request

OpenTelemetryCollector config files are different from reference #806

Closed

freschri requested review from parkand1, shapirov103, youngjeong46, elamaran11 and a team as code owners August 7, 2023 06:28

freschri mentioned this pull request Aug 7, 2023

Flux update #794

Merged

freschri changed the title ~~FluxCD fails to deploy multiple Kustomizations following PR #794~~ OpenTelemetryCollector config files are different from reference Aug 7, 2023

shapirov103 reviewed Aug 8, 2023

View reviewed changes

shapirov103 approved these changes Aug 8, 2023

View reviewed changes

aws-ia-ci requested changes Aug 8, 2023

View reviewed changes

shapirov103 requested changes Aug 8, 2023

View reviewed changes

elamaran11 approved these changes Aug 8, 2023

View reviewed changes

Merge branch 'main' into 806-opentelemetrycollector-config-files-are-…

cdfb562

…different-from-reference

elamaran11 requested changes Aug 9, 2023

View reviewed changes

added "apiserver" job with enablement flag, as requested in PR comments

ba537ff

elamaran11 reviewed Aug 11, 2023

View reviewed changes

addressed PR comments

71341e1

aws-ia-ci approved these changes Aug 16, 2023

View reviewed changes

shapirov103 approved these changes Aug 16, 2023

View reviewed changes

elamaran11 approved these changes Aug 16, 2023

View reviewed changes

elamaran11 merged commit 7133d40 into main Aug 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenTelemetryCollector config files are different from reference #810

OpenTelemetryCollector config files are different from reference #810

freschri commented Aug 7, 2023 •

edited

Loading

shapirov103 commented Aug 8, 2023

shapirov103 left a comment

shapirov103 commented Aug 8, 2023

aws-ia-ci left a comment

shapirov103 commented Aug 8, 2023

shapirov103 left a comment

elamaran11 left a comment

elamaran11 commented Aug 8, 2023

freschri commented Aug 8, 2023

elamaran11 left a comment •

edited

Loading

elamaran11 left a comment

freschri commented Aug 11, 2023

elamaran11 commented Aug 11, 2023

freschri commented Aug 11, 2023

elamaran11 commented Aug 11, 2023

freschri commented Aug 11, 2023

freschri commented Aug 11, 2023

elamaran11 left a comment

elamaran11 Aug 11, 2023

freschri Aug 11, 2023

elamaran11 Aug 11, 2023

shapirov103 Aug 16, 2023

elamaran11 commented Aug 15, 2023

freschri commented Aug 15, 2023

elamaran11 commented Aug 15, 2023

elamaran11 commented Aug 15, 2023

shapirov103 commented Aug 16, 2023

aws-ia-ci left a comment

shapirov103 left a comment

shapirov103 commented Aug 16, 2023

elamaran11 left a comment

elamaran11 commented Aug 16, 2023

OpenTelemetryCollector config files are different from reference #810

OpenTelemetryCollector config files are different from reference #810

Conversation

freschri commented Aug 7, 2023 • edited Loading

shapirov103 commented Aug 8, 2023

shapirov103 left a comment

Choose a reason for hiding this comment

shapirov103 commented Aug 8, 2023

aws-ia-ci left a comment

Choose a reason for hiding this comment

shapirov103 commented Aug 8, 2023

shapirov103 left a comment

Choose a reason for hiding this comment

elamaran11 left a comment

Choose a reason for hiding this comment

elamaran11 commented Aug 8, 2023

freschri commented Aug 8, 2023

elamaran11 left a comment • edited Loading

Choose a reason for hiding this comment

elamaran11 left a comment

Choose a reason for hiding this comment

freschri commented Aug 11, 2023

elamaran11 commented Aug 11, 2023

freschri commented Aug 11, 2023

elamaran11 commented Aug 11, 2023

freschri commented Aug 11, 2023

freschri commented Aug 11, 2023

elamaran11 left a comment

Choose a reason for hiding this comment

elamaran11 Aug 11, 2023

Choose a reason for hiding this comment

freschri Aug 11, 2023

Choose a reason for hiding this comment

elamaran11 Aug 11, 2023

Choose a reason for hiding this comment

shapirov103 Aug 16, 2023

Choose a reason for hiding this comment

elamaran11 commented Aug 15, 2023

freschri commented Aug 15, 2023

elamaran11 commented Aug 15, 2023

elamaran11 commented Aug 15, 2023

shapirov103 commented Aug 16, 2023

aws-ia-ci left a comment

Choose a reason for hiding this comment

shapirov103 left a comment

Choose a reason for hiding this comment

shapirov103 commented Aug 16, 2023

elamaran11 left a comment

Choose a reason for hiding this comment

elamaran11 commented Aug 16, 2023

freschri commented Aug 7, 2023 •

edited

Loading

elamaran11 left a comment •

edited

Loading