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.

Sign up for GitHub

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

Jump to bottom

add telemetry to hocon file #2

Merged
merged 1 commit into from
Jul 11, 2023
Merged

add telemetry to hocon file #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ locals {

app_name = "lake-loader-azure"
# TODO: Change once 0.1.0 is published
app_version = "0.1.0-rc2"
app_version = "0.1.0-rc3"

local_tags = {
Name = var.name
Expand Down Expand Up @@ -202,6 +202,13 @@ locals {
storage_account_name = var.storage_account_name
storage_container_name = var.storage_container_name
storage_container_path = var.storage_container_path

telemetry_disable = !var.telemetry_enabled
telemetry_collector_uri = join("", module.telemetry.*.collector_uri)
telemetry_user_provided_id = var.user_provided_id
telemetry_auto_gen_id = join("", module.telemetry.*.auto_generated_id)
telemetry_module_name = local.module_name
telemetry_module_version = local.module_version
})

user_data = templatefile("${path.module}/templates/user-data.sh.tmpl", {
Expand Down
10 changes: 10 additions & 0 deletions templates/config.hocon.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,14 @@
}
}
}

"telemetry": {
"disable": ${telemetry_disable}
"collectorUri": "${telemetry_collector_uri}"
"userProvidedId": "${telemetry_user_provided_id}"
"autoGeneratedId": "${telemetry_auto_gen_id}"
"moduleName": "${telemetry_module_name}"
"moduleVersion": "${telemetry_module_version}"
"instanceId": \$${INSTANCE_ID}
}
}
4 changes: 2 additions & 2 deletions templates/user-data.sh.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ sudo docker run \
--env AZURE_TENANT_ID=${tenant_id} \
--env AZURE_CLIENT_ID=${client_id} \
--env AZURE_CLIENT_SECRET=${client_secret} \
--env 'JAVA_OPTS=-Dconfig.override_with_env_vars=true ${java_opts}' \
--env "CONFIG_FORCE_telemetry_instanceId=$(get_instance_id)" \
--env INSTANCE_ID=$(get_instance_id) \
Copy link
Member

Choose a reason for hiding this comment

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

Can we please validate that this does indeed work? I tried this way initially and the reason I went for the force option is that it wasn't correctly seeding into the config or the tracking that was coming out.

Copy link

@colmsnowplow colmsnowplow Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think I was too hasty merging, apologies I misunderstood where we were up to with this and was too eager to press forward.

Bear with me though I encountered and solved a possibly related issue in another PR. Let me test what you describe here and circle back

--env JDK_JAVA_OPTIONS='${java_opts}' \
snowplow/lake-loader-azure:${version} \
--iglu-config /snowplow/config/iglu_config.json \
--config /snowplow/config/config.hocon
Expand Down
2 changes: 1 addition & 1 deletion variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ variable "tags" {

variable "java_opts" {
description = "Custom JAVA Options"
default = "-Dorg.slf4j.simpleLogger.defaultLogLevel=info -XX:MinRAMPercentage=50 -XX:MaxRAMPercentage=75"
default = "-XX:InitialRAMPercentage=75 -XX:MaxRAMPercentage=75"
Copy link

@colmsnowplow colmsnowplow Jul 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If I'm understanding you, this should be the default java opts for all (java runtime) app modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. These memory settings are more appropriate for how we run a single app per vm. And the log level setting is already the slf4j default, so need to add it here again explicitly.

Copy link
Contributor Author

@istreeter istreeter Jul 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In my opinion, some of the other changes I made here should also be how we do things in all other terraform modules:

  • Change JAVA_OPTS to JDK_JAVA_OPTIONS, which is the more standard env var and is compatible with our distroless images.
  • Template instanceId into the hocon file using an environment variable. So that all configuration comes from the same place.
  • Stop using -Dconfig.override_with_env_vars=true and CONFIG_FORCE_* which I find a bit hacky.

None of this is critical. But in my opinion it makes things a bit neater.

type = string
}

Expand Down