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

Trace more SASL calls #4510

Closed
wants to merge 1 commit into from
Closed

Trace more SASL calls #4510

wants to merge 1 commit into from

Conversation

tiran
Copy link
Contributor

@tiran tiran commented Dec 18, 2020

Add log trace for sasl_server_start() and sasl_server_step() to
investigate performance issues with SASL bind.

See: #4506
Signed-off-by: Christian Heimes cheimes@redhat.com

Add log trace for sasl_server_start() and sasl_server_step() to
investigate performance issues with SASL bind.

sasl_errstring() perform a simple and fast switch case mapping from
error code to const string.

See: 389ds#4506
Signed-off-by: Christian Heimes <cheimes@redhat.com>
rc = sasl_server_step(sasl_conn,
cred->bv_val, cred->bv_len,
&sdata, &slen);
rc = ids_sasl_server_step(pb_conn, cred, &sdata, &slen);
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on what we want to do here, we could add these in #ifdef DEBUG.

@mreynolds389
Copy link
Contributor

This is somewhat off topic and directed at the Dev team, but we have a problem with Trace Function Call logging level. It basically makes the server unusable. The server start up will fail because it times in when this logging level is enabled, and same for other processes. It's been over-used in the source, and I do not what to use that logging level in this PR.

@tiran maybe in this case connection logging is the next best choice (SLAPI_LOG_CONNS).

For Dev, we either need new logging levels or we need to change/remove some of the current "trace function call" logging statements. They need to be reduced, one way or another...

@Firstyear
Copy link
Contributor

@mreynolds389 Well, we need to do a lot related to logging in general, but this is a good point for certain.

@mreynolds389
Copy link
Contributor

@tiran if you change the log level from SLAPI_LOG_TRACE to SLAPI_LOG_CONNS you get my ack.

@Firstyear I don't think we should #ifdef this. This logging could be useful in customer situations, so it should be available with the standard build.

sasl_conn_t *sasl_conn;

slapi_log_err(SLAPI_LOG_TRACE, "ids_sasl_server_start", "=> (mech=%s)\n",
mech);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also log the connection ID in the logging (before and after):

slapi_log_err(SLAPI_LOG_CONNS, "ids_sasl_server_start", "=> (conn=%" PRIu64 " mech=%s)\n",
              conn->c_connid, mech);
...
slapi_log_err(SLAPI_LOG_CONNS, "ids_sasl_server_start", "<= (conn=%" PRIu64 " rc=%s)\n",
              conn->c_connid, sasl_errstring(rc, NULL, NULL));

@mreynolds389
Copy link
Contributor

I took this PR and created a new one with more logging improvements: #4749

I'm going to close this PR in favor of the new one. Thanks @tiran !

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.

None yet

3 participants