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

add capability to set an instance identifier which labels all metrics #1198

Merged
merged 10 commits into from
Apr 24, 2024
Merged

add capability to set an instance identifier which labels all metrics #1198

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
580158d
use latest version of observability with otlp updates, reduce export …
gvelez17 Apr 19, 2024
a2ce21b
for cas server, set an instance identifier from the ecs container
gvelez17 Apr 22, 2024
15d1c06
tell the linter that it will be a string, for reals
gvelez17 Apr 22, 2024
6d4482e
the linter wants to be very careful about regexp logic maybe
gvelez17 Apr 22, 2024
06c77db
fix typos
gvelez17 Apr 22, 2024
e333dff
do not escape / inside character class
gvelez17 Apr 22, 2024
1d4ad0d
add test coverage for some lines
gvelez17 Apr 22, 2024
f1b7fec
add a test for presence of the instanceIdentifier
gvelez17 Apr 22, 2024
55ff56d
set the instance identifier in the deployment, use in the code
gvelez17 Apr 24, 2024
6cafafe
make tests reflect expected reality
gvelez17 Apr 24, 2024
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
1 change: 1 addition & 0 deletions config/default.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
"collectorHost": "",
"exportIntervalMillis": 30000,
"exportTimeoutMillis": 20000,
"instanceIdentifier": "",
"prometheusPort": 0
},
"db": {
Expand Down
1 change: 1 addition & 0 deletions config/env/dev.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
"traceRatio": "@@METRICS_TRACE_RATIO",
"exportIntervalMillis": "@@METRICS_EXPORT_INTERVAL_MS",
"exportTimeoutMillis": "@@METRICS_EXPORT_TIMEOUT_MS",
"instanceIdentifier": "@@ECS_CONTAINER_METADATA_URI",
3benbox marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to inject this through the Dockerfile via an ENTRYPOINT script and call it INSTANCE_ID or something like that.

We do have AWS specific stuff in the env vars and code but parsing a very specific ECS env var directly in the code seems undesirable.

entrypoint.sh:

#!/bin/bash

uri=$ECS_CONTAINER_METADATA_URI

# Parse the last portion of the URI
value=${uri##*/}

# Remove everything after the hyphen to get the task ID
value=${value%%-*}

export INSTANCE_ID=$value

exec "$@"

Dockerfile:

.
.
.
WORKDIR /

COPY entrypoint.sh .
RUN chmod +x ./entrypoint.sh

COPY runner.sh .

ENTRYPOINT ["./entrypoint.sh"]
CMD [ "./runner.sh" ]

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have AWS specific stuff in the env vars and code but parsing a very specific ECS env var directly in the code seems undesirable.

Oh yeah, good call!

"prometheusPort": "@@METRICS_PORT"
},
"db": {
Expand Down
1 change: 1 addition & 0 deletions config/env/prod.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
"traceRatio": "@@METRICS_TRACE_RATIO",
"exportIntervalMillis": "@@METRICS_EXPORT_INTERVAL_MS",
"exportTimeoutMillis": "@@METRICS_EXPORT_TIMEOUT_MS",
"instanceIdentifier": "@@ECS_CONTAINER_METADATA_URI",
"prometheusPort": "@@METRICS_PORT"
},
"db": {
Expand Down
1 change: 1 addition & 0 deletions config/env/test.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"traceRatio": "@@METRICS_TRACE_RATIO",
"exportIntervalMillis": "@@METRICS_EXPORT_INTERVAL_MS",
"exportTimeoutMillis": "@@METRICS_EXPORT_TIMEOUT_MS",
"instanceIdentifier": "@@ECS_CONTAINER_METADATA_URI",
"prometheusPort": "@@METRICS_PORT"
},
"db": {
Expand Down
14 changes: 7 additions & 7 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
"@ceramicnetwork/common": "^2.28.0-rc.0",
"@ceramicnetwork/core": "^2.35.0-rc.0",
"@ceramicnetwork/logger": "^2.5.0",
"@ceramicnetwork/observability": "^1.4.6",
"@ceramicnetwork/observability": "^1.5.0",
"@ceramicnetwork/streamid": "^2.15.0-rc.0",
"@ceramicnetwork/wasm-bloom-filter": "^0.1.0",
"@overnightjs/core": "^1.7.6",
Expand Down
39 changes: 39 additions & 0 deletions src/__tests__/ceramic_integration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,3 +584,42 @@ describe('CAR file', () => {
await casIPFS.stop()
})
})

describe('Metrics Options', () => {
test('cas starts with a typical instance identifier', async () => {
const ipfsApiPort = await getPort()
const casIPFS = await createIPFS(ipfsApiPort)
const ganacheServer = await makeGanache()
const dbConnection = await createDbConnection()
const casPort = await getPort()
const cas = await makeCAS(createInjector(), dbConnection, {
mode: 'server',
ipfsPort: ipfsApiPort,
ganachePort: ganacheServer.port,
port: casPort,
useSmartContractAnchors: true,
metrics: {
instanceIdentifier: 'http://127.0.0.1/v3/234fffffffffffffffffffffffffffffffff9726129'
}
})
await cas.start()
// Teardown
await cas.stop()

const cas2 = await makeCAS(createInjector(), dbConnection, {
mode: 'server',
ipfsPort: ipfsApiPort,
ganachePort: ganacheServer.port,
port: casPort,
useSmartContractAnchors: true,
metrics: {
instanceIdentifier: 'fred'
}
})
await cas2.start()
await cas2.stop()

await ganacheServer.close()
await casIPFS.stop()
})
})
12 changes: 12 additions & 0 deletions src/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,18 @@ export class CeramicAnchorApp {
)
Metrics.count('HELLO', 1)
logger.imp('Metrics exporter started')
if (this.config.metrics.instanceIdentifier) {
// In the API server, the instanceIdentifer is set from ECS_CONTAINER_METADATA_URI
Copy link
Contributor

Choose a reason for hiding this comment

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

If done using the entrypoint.sh script above, we won't need to parse this in the code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right! the logic isn't quite the same - not sure if it matters, i was defaulting to the full uri if there wasn't a task id at the end - but i agree yours is better, since it moves it to the deployment and therefore we can make it very specific to what we expect!

// example value: http://169.254.170.2/v3/234fae8d117d4b76a7af0600b94fc195-2439726129

// in this case, return only the task id following the last /
const match = this.config.metrics.instanceIdentifier.match(/\/([^/]*)$/);
if (match && match[1]) {
Metrics.setInstanceIdentifier(match[1])
} else {
Metrics.setInstanceIdentifier(this.config.metrics.instanceIdentifier)
}
}
} catch (e: any) {
logger.imp('ERROR: Metrics exporter failed to start. Continuing anyway.')
logger.err(e)
Expand Down
Loading