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

Handle warnings from task manager without stack traces. #30692

Merged
merged 4 commits into from
Feb 12, 2019
Merged

Handle warnings from task manager without stack traces. #30692

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c1ec3bd
Handle warnings from task manager without stack traces.
njd5475 Feb 11, 2019
354e8c6
Fix typescript errors.
njd5475 Feb 11, 2019
e55442e
Catch no living connection errors.
njd5475 Feb 11, 2019
052fc54
Should use string substitution instead of concatenation.
njd5475 Feb 11, 2019
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
18 changes: 11 additions & 7 deletions x-pack/plugins/maps/server/maps_telemetry/telemetry_task.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,16 @@ export const TASK_ID = `Maps-${TELEMETRY_TASK_TYPE}`;
export function scheduleTask(server, taskManager) {
const { kbnServer } = server.plugins.xpack_main.status.plugin;

kbnServer.afterPluginsInit(() => {
taskManager.schedule({
id: TASK_ID,
taskType: TELEMETRY_TASK_TYPE,
state: { stats: {}, runs: 0 },
});
kbnServer.afterPluginsInit(async () => {
try {
await taskManager.schedule({
id: TASK_ID,
taskType: TELEMETRY_TASK_TYPE,
state: { stats: {}, runs: 0 },
});
}catch(e) {
server.log(['warning', 'maps'], `Error scheduling telemetry task, received ${e.message}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple things:

  1. This is using the tag maps, and I assume it should be using a tag relevant to task manager.
  2. I don't think swallowing all possible exceptions and treating them as warnings is the right approach here, or pretty much ever. There are specific errors we'd expect to see in the course of normal operation (like Elasticsearch not being available or authentication being rejected), but if something truly exceptional occurs, we'll want it to be treated properly as an error.

Copy link
Member

Choose a reason for hiding this comment

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

  1. this is the maps collector, so that part is right
  2. I agree this shouldn't be swallowed, but on the other hand, I hope the timing errors or connection handling can be handled more by Task Manager

Copy link
Contributor Author

@njd5475 njd5475 Feb 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The timing errors should be avoidable, by reworking the way the task manager gets initialized. So that we completely avoid the chicken or the egg problem. But I agree that it would be nice to handle these inside the task manager rather than put this on the consumers of the task manager service.

}
});
}

Expand Down Expand Up @@ -70,4 +74,4 @@ export function getNextMidnight() {
nextMidnight.setHours(0, 0, 0, 0);
nextMidnight.setDate(nextMidnight.getDate() + 1);
return nextMidnight.toISOString();
}
}
1 change: 1 addition & 0 deletions x-pack/plugins/oss_telemetry/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,5 @@ export interface HapiServer {
config: () => {
get: (prop: string) => any;
};
log: (context: string[], message: string) => void;
}
16 changes: 10 additions & 6 deletions x-pack/plugins/oss_telemetry/server/lib/tasks/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,15 @@ export function scheduleTasks(server: HapiServer) {
const { taskManager } = server;
const { kbnServer } = server.plugins.xpack_main.status.plugin;

kbnServer.afterPluginsInit(() => {
taskManager.schedule({
id: `${PLUGIN_ID}-${VIS_TELEMETRY_TASK}`,
taskType: VIS_TELEMETRY_TASK,
state: { stats: {}, runs: 0 },
});
kbnServer.afterPluginsInit(async () => {
try {
await taskManager.schedule({
id: `${PLUGIN_ID}-${VIS_TELEMETRY_TASK}`,
taskType: VIS_TELEMETRY_TASK,
state: { stats: {}, runs: 0 },
});
} catch (e) {
server.log(['warning', 'telemetry'], `Error scheduling task, received ${e.message}`);
}
});
}
1 change: 1 addition & 0 deletions x-pack/plugins/oss_telemetry/test_utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,5 @@ export const getMockKbnServer = (
},
},
config: () => ({ get: () => '' }),
log: () => undefined,
});
30 changes: 17 additions & 13 deletions x-pack/plugins/reporting/server/lib/validate_max_content_length.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,20 +12,24 @@ export async function validateMaxContentLength(server: any, log: (message: strin
const config = server.config();
const { callWithInternalUser } = server.plugins.elasticsearch.getCluster('data');

const elasticClusterSettingsResponse = await callWithInternalUser('cluster.getSettings', {
includeDefaults: true,
});
const { persistent, transient, defaults: defaultSettings } = elasticClusterSettingsResponse;
const elasticClusterSettings = defaults({}, persistent, transient, defaultSettings);
try {
const elasticClusterSettingsResponse = await callWithInternalUser('cluster.getSettings', {
includeDefaults: true,
});
const { persistent, transient, defaults: defaultSettings } = elasticClusterSettingsResponse;
const elasticClusterSettings = defaults({}, persistent, transient, defaultSettings);

const elasticSearchMaxContent = get(elasticClusterSettings, 'http.max_content_length', '100mb');
const elasticSearchMaxContentBytes = numeral().unformat(elasticSearchMaxContent.toUpperCase());
const kibanaMaxContentBytes = config.get(KIBANA_MAX_SIZE_BYTES_PATH);
const elasticSearchMaxContent = get(elasticClusterSettings, 'http.max_content_length', '100mb');
const elasticSearchMaxContentBytes = numeral().unformat(elasticSearchMaxContent.toUpperCase());
const kibanaMaxContentBytes = config.get(KIBANA_MAX_SIZE_BYTES_PATH);

if (kibanaMaxContentBytes > elasticSearchMaxContentBytes) {
log(
`${KIBANA_MAX_SIZE_BYTES_PATH} (${kibanaMaxContentBytes}) is higher than ElasticSearch's ${ES_MAX_SIZE_BYTES_PATH} (${elasticSearchMaxContentBytes}). ` +
`Please set ${ES_MAX_SIZE_BYTES_PATH} in ElasticSearch to match, or lower your ${KIBANA_MAX_SIZE_BYTES_PATH} in Kibana to avoid this warning.`
);
if (kibanaMaxContentBytes > elasticSearchMaxContentBytes) {
log(
`${KIBANA_MAX_SIZE_BYTES_PATH} (${kibanaMaxContentBytes}) is higher than ElasticSearch's ${ES_MAX_SIZE_BYTES_PATH} (${elasticSearchMaxContentBytes}). ` +
`Please set ${ES_MAX_SIZE_BYTES_PATH} in ElasticSearch to match, or lower your ${KIBANA_MAX_SIZE_BYTES_PATH} in Kibana to avoid this warning.`
);
}
} catch (e) {
log(`Could not retrieve cluster settings, because of ${e.message}`);
}
}
2 changes: 1 addition & 1 deletion x-pack/plugins/task_manager/task_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export class TaskManager {
})
.catch((err: Error) => {
// FIXME: check the type of error to make sure it's actually an ES error
logger.warning(err.message);
logger.warning(`PollError ${err.message}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This sort of thing should be handled by a logger tag rather than string concatenation. This ensures folks can reliably filter/drill-down alerts based on type.


// rety again to initialize store and poller, using the timing of
// task_manager's configurable poll interval
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/task_manager/task_pool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ export class TaskPool {
task
.run()
.catch(err => {
this.logger.warning(`Task ${task} failed in attempt to run: ${err}`);
this.logger.warning(`Task ${task} failed in attempt to run: ${err.message}`);
})
.then(() => this.running.delete(task));
}
Expand Down